Re: [PATCH net-next] bridge: ebtables: Avoid resetting limit rule state
On Fri, Dec 08, 2017 at 06:46:06AM +0100, Linus Lüssing wrote:
> Extending the usersize to include info->prev would probably be too
> hackish/ugly, right?
And wouldn't be enough anyway, since
info->{credit,credit_cap,cost} would still be zeroed... Hm.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next] bridge: ebtables: Avoid resetting limit rule state
On Thu, Dec 07, 2017 at 01:26:19AM +0100, Pablo Neira Ayuso wrote:
> > I also had a quick look at a 4.15-rc1 kernel in a VM now. I still
> > end up in ebt_limit_mt_check() with the variables being reset
> > when editing the table somewhere.
>
> My question is if your fix would work with 4.15-rc1.
You are absoluetly right, it's not working anymore since the
commit you mentioned initially :-(
("xtables: extend matches and targets with .usersize").
info->prev is always 0 since exactly this commit.
That means, trying tricks in ebt_limit_mt_check() is too
late now, the old values are already overwritten? (or is there
some commit scheme which installs the ebt_limit_info provided
by ebt_limit_check() some time after its call?)
Extending the usersize to include info->prev would probably be too
hackish/ugly, right?
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Bridge] [PATCH net-next] bridge: ebtables: Avoid resetting limit rule state
On Mon, Dec 04, 2017 at 05:53:35AM +0100, Linus Lüssing wrote: > And so, no I do not have this patch. I looked at it now, but it > does not seem to have any relation with .matchinfo, does it? Relation between .usersize and .checkentry I ment, not .usersize and .matchinfo. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next] bridge: ebtables: Avoid resetting limit rule state
Hi Pablo,
Thanks for your reply!
On Tue, Nov 28, 2017 at 12:30:08AM +0100, Pablo Neira Ayuso wrote:
> [...]
> > diff --git a/net/bridge/netfilter/ebt_limit.c
> > b/net/bridge/netfilter/ebt_limit.c
> > index 61a9f1be1263..f74b48633feb 100644
> > --- a/net/bridge/netfilter/ebt_limit.c
> > +++ b/net/bridge/netfilter/ebt_limit.c
> > @@ -69,6 +69,10 @@ static int ebt_limit_mt_check(const struct
> > xt_mtchk_param *par)
> > {
> > struct ebt_limit_info *info = par->matchinfo;
> >
> > + /* Do not reset state on unrelated table changes */
> > + if (info->prev)
> > + return 0;
>
> What kernel version are you using? I suspect you don't have this
> applied?
I'm indeed using a 4.4.102 kernel, as LEDE is still in the process
of updating to 4.14. So 4.4 with LEDE is where I got the measurement
results from.
>
> commit ec23189049651b16dc2ffab35a4371dc1f491aca
> Author: Willem de Bruijn
> Date: Mon Jan 2 17:19:46 2017 -0500
>
> xtables: extend matches and targets with .usersize
And so, no I do not have this patch. I looked at it now, but it
does not seem to have any relation with .matchinfo, does it?
I also had a quick look at a 4.15-rc1 kernel in a VM now. I still
end up in ebt_limit_mt_check() with the variables being reset
when editing the table somewhere.
Regards, Linus
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH net-next] bridge: ebtables: Avoid resetting limit rule state
So far any changes with ebtables will reset the state of limit rules,
leading to spikes in traffic. This is especially noticeable if changes
are done frequently, for instance via a daemon.
This patch fixes this by bailing out from (re)setting if the limit
rule was initialized before.
When sending packets every 250ms for 600s, with a
"--limit 1/sec --limit-burst 50" rule and a command like this
in the background:
$ ebtables -N VOIDCHAIN
$ while true; do ebtables -F VOIDCHAIN; sleep 30; done
The results are:
Before: ~1600 packets
After: 650 packets
Signed-off-by: Linus Lüssing <linus.luess...@c0d3.blue>
---
net/bridge/netfilter/ebt_limit.c | 4
1 file changed, 4 insertions(+)
diff --git a/net/bridge/netfilter/ebt_limit.c b/net/bridge/netfilter/ebt_limit.c
index 61a9f1be1263..f74b48633feb 100644
--- a/net/bridge/netfilter/ebt_limit.c
+++ b/net/bridge/netfilter/ebt_limit.c
@@ -69,6 +69,10 @@ static int ebt_limit_mt_check(const struct xt_mtchk_param
*par)
{
struct ebt_limit_info *info = par->matchinfo;
+ /* Do not reset state on unrelated table changes */
+ if (info->prev)
+ return 0;
+
/* Check for overflow. */
if (info->burst == 0 ||
user2credits(info->avg * info->burst) < user2credits(info->avg)) {
--
2.11.0
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH net v3] bridge: ebtables: fix reception of frames DNAT-ed to bridge device/port
When trying to redirect bridged frames to the bridge device itself or
a bridge port (brouting) via the dnat target then this currently fails:
The ethernet destination of the frame is dnat'ed to the MAC address of
the bridge device or port just fine. However, the IP code drops it in
the beginning of ip_input.c/ip_rcv() as the dnat target left
the skb->pkt_type as PACKET_OTHERHOST.
Fixing this by resetting skb->pkt_type to an appropriate type after
dnat'ing.
Signed-off-by: Linus Lüssing <linus.luess...@c0d3.blue>
---
Changelog v3:
- moved pkt_type fixup into ebtable dnat code
-> v1/v2 only fixed it for prerouting/dnat so far, now tested
and verified that v3 fixes it for brouting/dnat, too
- updated commit message
Changelog v2:
- refrain from altering pkt_type for multicast packets
with a unicast destination MAC
---
net/bridge/netfilter/ebt_dnat.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/net/bridge/netfilter/ebt_dnat.c b/net/bridge/netfilter/ebt_dnat.c
index 4e0b0c3..21acb53 100644
--- a/net/bridge/netfilter/ebt_dnat.c
+++ b/net/bridge/netfilter/ebt_dnat.c
@@ -9,6 +9,7 @@
*/
#include
#include
+#include "../br_private.h"
#include
#include
#include
@@ -18,11 +19,32 @@ static unsigned int
ebt_dnat_tg(struct sk_buff *skb, const struct xt_action_param *par)
{
const struct ebt_nat_info *info = par->targinfo;
+ struct net_device *dev;
if (!skb_make_writable(skb, 0))
return EBT_DROP;
ether_addr_copy(eth_hdr(skb)->h_dest, info->mac);
+
+ if (is_multicast_ether_addr(info->mac)) {
+ if (is_broadcast_ether_addr(info->mac))
+ skb->pkt_type = PACKET_BROADCAST;
+ else
+ skb->pkt_type = PACKET_MULTICAST;
+ } else {
+ rcu_read_lock();
+ if (xt_hooknum(par) != NF_BR_BROUTING)
+ dev = br_port_get_rcu(xt_in(par))->br->dev;
+ else
+ dev = xt_in(par);
+
+ if (ether_addr_equal(info->mac, dev->dev_addr))
+ skb->pkt_type = PACKET_HOST;
+ else
+ skb->pkt_type = PACKET_OTHERHOST;
+ rcu_read_unlock();
+ }
+
return info->target;
}
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] bridge: ebtables: fix reception of frames DNAT-ed to bridge device
On Tue, Mar 21, 2017 at 04:32:45PM -0700, Stephen Hemminger wrote: > On Tue, 21 Mar 2017 23:28:45 +0100 > Linus Lüssing <linus.luess...@c0d3.blue> wrote: > > > However, the IP code drops it in the beginning of ip_input.c/ip_rcv() > > as the dnat target did not update the skb->pkt_type. If after > > dnat'ing the packet is now destined to us then the skb->pkt_type > > needs to be updated from PACKET_OTHERHOST to PACKET_HOST, too. > > Why not fix DNAT netfilter module rather than hacking bridge code here. Sorry for the late response. Wanted to do some more testing before replying. My assumptions regarding macvlan were wrong: A) The code my patch adds is not touched in the case of a macvlan device on top of a bridge - with macvlan there seems to be no FDB entry for the MAC address of the macvlan device at all, actually, resulting in the flooding of a frame intended for that device (is this, ehm, known/intended?). B) ip_rcv() does not drop for a macvlan device, because macvlan unconditionally sets skb->pkt_type to PACKET_HOST in macvlan_handle_frame(). (And I guess, maybe I shouldn't actually care that much about macvlans on top of a bridge, as a veth-pair is much cleaner for that?) Regarding netdev::dev_addrs, if I understand the kernel code correctly, it is only, potentially populated with more than netdev::dev_addr for two drivers, via dev_addr_add(): bnx2x and ixgbe. So for a bridge device, there should never be more than one item in netdev::dev_addrs (= netdev::dev_addr), correct? So, currently I'm happy with moving the fix into the ebtables dnat code and just checking against the netdev::dev_addr of the bridge device now. (if anyone has any suggestions regarding upper devices I should test with that, then please let me know) (Sorry, Pablo, for me pressing against your earlier suggestion to put the fix in the ebtables dnat instead of bridge code. :( ) Regards, Linus -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] bridge: ebtables: fix reception of frames DNAT-ed to bridge device
When trying to redirect bridged frames to the bridge device itself
via the ebtables nat-prerouting chain and the dnat target then this
currently fails:
The ethernet destination of the frame is dnat'ed to the MAC address of
the bridge itself just fine and the correctly altered frame can even
be captured via a tcpdump on br0 (with or without promisc mode).
However, the IP code drops it in the beginning of ip_input.c/ip_rcv()
as the dnat target did not update the skb->pkt_type. If after
dnat'ing the packet is now destined to us then the skb->pkt_type
needs to be updated from PACKET_OTHERHOST to PACKET_HOST, too.
Signed-off-by: Linus Lüssing <linus.luess...@c0d3.blue>
---
Changelog v2:
* refrain from altering pkt_type for multicast packets
with a unicast destination MAC
---
net/bridge/br_input.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 013f2290b..fd7bc4c 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -198,8 +198,13 @@ int br_handle_frame_finish(struct net *net, struct sock
*sk, struct sk_buff *skb
if (dst) {
unsigned long now = jiffies;
- if (dst->is_local)
+ if (dst->is_local) {
+ /* fix up potential DNAT inconsistencies */
+ if (skb->pkt_type == PACKET_OTHERHOST)
+ skb->pkt_type = PACKET_HOST;
+
return br_pass_frame_up(skb);
+ }
if (now != dst->used)
dst->used = now;
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Bridge] [PATCH net] bridge: ebtables: fix reception of frames DNAT-ed to bridge device
On Sun, Mar 19, 2017 at 05:55:06PM +0100, Linus Lüssing wrote:
> On Fri, Mar 17, 2017 at 02:10:44PM +0100, Pablo Neira Ayuso wrote:
> > Wait.
> >
> > May this break local multicast listener that are bound to the bridge
> > interface? Assuming the bridge interface got an IP address, and that
> > there is local multicast listener.
> >
> > Missing anything here?
>
> Hm, for multicast packets usually the code path a few lines
> later in br_handle_frame_finish() should be taken instead.
>
> But you might be right for IP multicast packets with a unicast MAC
> destination (due to whatever reason, for instance via DNAT'ing
> again).
>
> Will check that - thanks!
Ok, I tested DNAT'ing an IP multicast packet to the unicast MAC address
of the bridge interface.
Both ping-ing to an IPv4 and IPv6 multicast listener on br0 worked
and was replied to fine, both with or without changing skb->pkt_type
from PACKET_MULTICAST to PACKET_HOST.
("$ ping 224.1.0.123" and "$ ping6 ff02::1:ff40:707c%in0" from a
network namespace, tied into the bridge via veth)
Also, a DNAT'ed PACKET_BROADCAST worked, with or without changing
it to PACKET_HOST.
I also checked via tcpdump that the destination MAC was changed
successfully.
So, so far I wasn't able to find any bugs with the current
patch. But I think I like the idea of leaving the skb->pkt_type
unaltered for PACKET_MULTICAST and PACKET_BROADCAST, seems cleaner.
I'd just add an "if (skb->pkt_type == PACKET_OTHERHOST)" check
then and resend a PATCH v2.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net] bridge: ebtables: fix reception of frames DNAT-ed to bridge device
On Fri, Mar 17, 2017 at 02:10:44PM +0100, Pablo Neira Ayuso wrote: > Wait. > > May this break local multicast listener that are bound to the bridge > interface? Assuming the bridge interface got an IP address, and that > there is local multicast listener. > > Missing anything here? Hm, for multicast packets usually the code path a few lines later in br_handle_frame_finish() should be taken instead. But you might be right for IP multicast packets with a unicast MAC destination (due to whatever reason, for instance via DNAT'ing again). Will check that - thanks! -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net] bridge: ebtables: fix reception of frames DNAT-ed to bridge device
On Wed, Mar 15, 2017 at 07:15:39PM +0100, Pablo Neira Ayuso wrote: > Could you update ebtables dnat to check if the ethernet address > matches the one of the input bridge interface, so we mangle the > ->pkt_type accordingly from there, instead of doing this from the > core? Actually, that was the approach I thought about and went for first (and it would probably work for me). Just checking against the bridge device's net_device::dev_addr. I scratched it though, as I was afraid that the issue might still exist for people using some other upper device on top of the bridge device. For instance, macvlan? And iterating over the net_device::dev_addrs list seemed too costly for fast path to me. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net] bridge: ebtables: fix reception of frames DNAT-ed to bridge device
On Wed, Mar 15, 2017 at 11:42:11AM +0100, Pablo Neira Ayuso wrote: > I'm missing then why redirect is not then just enough for Linus usecase. For my usecase, the MAC address is configured by the user from a Web-UI. It may or may not be the one from the bridge device. Besides, found it counter intuitive that DNAT did not work here and took me some time to find out why. At least I didn't read about any such known limitations of the dnat target in the ebtables manpage. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH net] bridge: ebtables: fix reception of frames DNAT-ed to bridge device
When trying to redirect bridged frames to the bridge device itself
via the ebtables nat-prerouting chain and the dnat target then this
currently fails:
The ethernet destination of the frame is dnat'ed to the MAC address of
the bridge itself just fine and the correctly altered frame can even
be captured via a tcpdump on br0 (with or without promisc mode).
However, the IP code drops it in the beginning of ip_input.c/ip_rcv()
as the dnat target did not update the skb->pkt_type. If after
dnat'ing the packet is now destined to us then the skb->pkt_type
needs to be updated from PACKET_OTHERHOST to PACKET_HOST, too.
Signed-off-by: Linus Lüssing <linus.luess...@c0d3.blue>
---
net/bridge/br_input.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 013f2290b..ec83175 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -198,8 +198,12 @@ int br_handle_frame_finish(struct net *net, struct sock
*sk, struct sk_buff *skb
if (dst) {
unsigned long now = jiffies;
- if (dst->is_local)
+ if (dst->is_local) {
+ /* fix up potential DNAT mess */
+ skb->pkt_type = PACKET_HOST;
+
return br_pass_frame_up(skb);
+ }
if (now != dst->used)
dst->used = now;
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
