| Message ID | 20220411133837.318876-2-troglobit@gmail.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: bridge: forwarding of unknown IPv4/IPv6/MAC BUM traffic | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/apply | fail | Patch does not apply to net-next |
On 11/04/2022 16:38, Joachim Wiberg wrote: > The bridge itself is also a port, but unfortunately it does not (yet) > have a 'struct net_bridge_port'. However, in many cases we want to > treat it as a proper port so concessions have been made, e.g., NULL > port or host_joined attributes. > > This patch is an attempt to more of the same by adding support for > controlling flooding of unknown broadcast/unicast/multicast to the > bridge. Something we often also want to control in an offloaded > switching fabric. > > Signed-off-by: Joachim Wiberg <troglobit@gmail.com> > --- > net/bridge/br_device.c | 4 ++++ > net/bridge/br_input.c | 11 ++++++++--- > net/bridge/br_private.h | 3 +++ > 3 files changed, 15 insertions(+), 3 deletions(-) > > diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c > index 8d6bab244c4a..0aa7d21ac82c 100644 > --- a/net/bridge/br_device.c > +++ b/net/bridge/br_device.c > @@ -526,6 +526,10 @@ void br_dev_setup(struct net_device *dev) > br->bridge_ageing_time = br->ageing_time = BR_DEFAULT_AGEING_TIME; > dev->max_mtu = ETH_MAX_MTU; > > + br_opt_toggle(br, BROPT_UNICAST_FLOOD, 1); This one must be false by default. It changes current default behaviour. Unknown unicast is not currently passed up to the bridge if the port is not in promisc mode, this will change it. You'll have to make it consistent with promisc (e.g. one way would be for promisc always to enable unicast flood and it won't be possible to be disabled while promisc). > + br_opt_toggle(br, BROPT_MCAST_FLOOD, 1); > + br_opt_toggle(br, BROPT_BCAST_FLOOD, 1); s/1/true/ for consistency > + > br_netfilter_rtable_init(br); > br_stp_timer_init(br); > br_multicast_init(br); > diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c > index 196417859c4a..d439b876bdf5 100644 > --- a/net/bridge/br_input.c > +++ b/net/bridge/br_input.c > @@ -118,7 +118,8 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb > /* by definition the broadcast is also a multicast address */ > if (is_broadcast_ether_addr(eth_hdr(skb)->h_dest)) { > pkt_type = BR_PKT_BROADCAST; > - local_rcv = true; > + if (br_opt_get(br, BROPT_BCAST_FLOOD)) > + local_rcv = true; > } else { > pkt_type = BR_PKT_MULTICAST; > if (br_multicast_rcv(&brmctx, &pmctx, vlan, skb, vid)) > @@ -161,12 +162,16 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb > } > mcast_hit = true; > } else { > - local_rcv = true; > - br->dev->stats.multicast++; > + if (br_opt_get(br, BROPT_MCAST_FLOOD)) { > + local_rcv = true; > + br->dev->stats.multicast++; > + } > } > break; > case BR_PKT_UNICAST: > dst = br_fdb_find_rcu(br, eth_hdr(skb)->h_dest, vid); > + if (!dst && br_opt_get(br, BROPT_UNICAST_FLOOD)) > + local_rcv = true; > break; This adds new tests for all fast paths for host traffic, especially the port - port communication which is the most critical one. Please at least move the unicast test to the "else" block of "if (dst)" later. The other tests can be moved to host only code too, but would require bigger changes. Please try to keep the impact on the fast-path at minimum. > default: > break; > diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h > index 18ccc3d5d296..683bd0ee4c64 100644 > --- a/net/bridge/br_private.h > +++ b/net/bridge/br_private.h > @@ -449,6 +449,9 @@ enum net_bridge_opts { > BROPT_VLAN_BRIDGE_BINDING, > BROPT_MCAST_VLAN_SNOOPING_ENABLED, > BROPT_MST_ENABLED, > + BROPT_UNICAST_FLOOD, > + BROPT_MCAST_FLOOD, > + BROPT_BCAST_FLOOD, > }; > > struct net_bridge {
On 12/04/2022 21:27, Nikolay Aleksandrov wrote: > On 11/04/2022 16:38, Joachim Wiberg wrote: >> The bridge itself is also a port, but unfortunately it does not (yet) >> have a 'struct net_bridge_port'. However, in many cases we want to >> treat it as a proper port so concessions have been made, e.g., NULL >> port or host_joined attributes. >> >> This patch is an attempt to more of the same by adding support for >> controlling flooding of unknown broadcast/unicast/multicast to the >> bridge. Something we often also want to control in an offloaded >> switching fabric. >> >> Signed-off-by: Joachim Wiberg <troglobit@gmail.com> >> --- >> net/bridge/br_device.c | 4 ++++ >> net/bridge/br_input.c | 11 ++++++++--- >> net/bridge/br_private.h | 3 +++ >> 3 files changed, 15 insertions(+), 3 deletions(-) >> >> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c >> index 8d6bab244c4a..0aa7d21ac82c 100644 >> --- a/net/bridge/br_device.c >> +++ b/net/bridge/br_device.c >> @@ -526,6 +526,10 @@ void br_dev_setup(struct net_device *dev) >> br->bridge_ageing_time = br->ageing_time = BR_DEFAULT_AGEING_TIME; >> dev->max_mtu = ETH_MAX_MTU; >> >> + br_opt_toggle(br, BROPT_UNICAST_FLOOD, 1); > > This one must be false by default. It changes current default behaviour. > Unknown unicast is not currently passed up to the bridge if the port is s/port/bridge/ in promisc mode > not in promisc mode, this will change it. You'll have to make it consistent > with promisc (e.g. one way would be for promisc always to enable unicast flood > and it won't be possible to be disabled while promisc). > >> + br_opt_toggle(br, BROPT_MCAST_FLOOD, 1); >> + br_opt_toggle(br, BROPT_BCAST_FLOOD, 1); > > s/1/true/ for consistency > >> + >> br_netfilter_rtable_init(br); >> br_stp_timer_init(br); >> br_multicast_init(br); >> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c >> index 196417859c4a..d439b876bdf5 100644 >> --- a/net/bridge/br_input.c >> +++ b/net/bridge/br_input.c >> @@ -118,7 +118,8 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb >> /* by definition the broadcast is also a multicast address */ >> if (is_broadcast_ether_addr(eth_hdr(skb)->h_dest)) { >> pkt_type = BR_PKT_BROADCAST; >> - local_rcv = true; >> + if (br_opt_get(br, BROPT_BCAST_FLOOD)) >> + local_rcv = true; >> } else { >> pkt_type = BR_PKT_MULTICAST; >> if (br_multicast_rcv(&brmctx, &pmctx, vlan, skb, vid)) >> @@ -161,12 +162,16 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb >> } >> mcast_hit = true; >> } else { >> - local_rcv = true; >> - br->dev->stats.multicast++; >> + if (br_opt_get(br, BROPT_MCAST_FLOOD)) { >> + local_rcv = true; >> + br->dev->stats.multicast++; >> + } >> } >> break; >> case BR_PKT_UNICAST: >> dst = br_fdb_find_rcu(br, eth_hdr(skb)->h_dest, vid); >> + if (!dst && br_opt_get(br, BROPT_UNICAST_FLOOD)) >> + local_rcv = true; >> break; > > This adds new tests for all fast paths for host traffic, > especially the port - port communication which is the most critical one. > Please at least move the unicast test to the "else" block of "if (dst)" later. > > The other tests can be moved to host only code too, but would require bigger changes. > Please try to keep the impact on the fast-path at minimum. > >> default: >> break; >> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h >> index 18ccc3d5d296..683bd0ee4c64 100644 >> --- a/net/bridge/br_private.h >> +++ b/net/bridge/br_private.h >> @@ -449,6 +449,9 @@ enum net_bridge_opts { >> BROPT_VLAN_BRIDGE_BINDING, >> BROPT_MCAST_VLAN_SNOOPING_ENABLED, >> BROPT_MST_ENABLED, >> + BROPT_UNICAST_FLOOD, >> + BROPT_MCAST_FLOOD, >> + BROPT_BCAST_FLOOD, >> }; >> >> struct net_bridge { >
On Tue, Apr 12, 2022 at 21:27, Nikolay Aleksandrov <razor@blackwall.org> wrote: > On 11/04/2022 16:38, Joachim Wiberg wrote: >> @@ -526,6 +526,10 @@ void br_dev_setup(struct net_device *dev) >> br->bridge_ageing_time = br->ageing_time = BR_DEFAULT_AGEING_TIME; >> dev->max_mtu = ETH_MAX_MTU; >> + br_opt_toggle(br, BROPT_UNICAST_FLOOD, 1); > This one must be false by default. It changes current default behaviour. > Unknown unicast is not currently passed up to the bridge if the port is > not in promisc mode, this will change it. You'll have to make it consistent > with promisc (e.g. one way would be for promisc always to enable unicast flood > and it won't be possible to be disabled while promisc). Ouch, my bad! Will look into how to let this have as little impact as possible. I like your semantics there, promisc should always win. >> + br_opt_toggle(br, BROPT_MCAST_FLOOD, 1); >> + br_opt_toggle(br, BROPT_BCAST_FLOOD, 1); > > s/1/true/ for consistency Of course, thanks! >> @@ -118,7 +118,8 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb >> /* by definition the broadcast is also a multicast address */ >> if (is_broadcast_ether_addr(eth_hdr(skb)->h_dest)) { >> pkt_type = BR_PKT_BROADCAST; >> - local_rcv = true; >> + if (br_opt_get(br, BROPT_BCAST_FLOOD)) >> + local_rcv = true; >> } else { >> pkt_type = BR_PKT_MULTICAST; >> if (br_multicast_rcv(&brmctx, &pmctx, vlan, skb, vid)) >> @@ -161,12 +162,16 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb >> } >> mcast_hit = true; >> } else { >> - local_rcv = true; >> - br->dev->stats.multicast++; >> + if (br_opt_get(br, BROPT_MCAST_FLOOD)) { >> + local_rcv = true; >> + br->dev->stats.multicast++; >> + } >> } >> break; >> case BR_PKT_UNICAST: >> dst = br_fdb_find_rcu(br, eth_hdr(skb)->h_dest, vid); >> + if (!dst && br_opt_get(br, BROPT_UNICAST_FLOOD)) >> + local_rcv = true; >> break; > > This adds new tests for all fast paths for host traffic, especially > the port - port communication which is the most critical one. Please > at least move the unicast test to the "else" block of "if (dst)" > later. OK, will fix! > The other tests can be moved to host only code too, but would require > bigger changes. Please try to keep the impact on the fast-path at > minimum. Interesting, you mean by speculatively setting local_rcv = true and postpone the decsion to br_pass_frame_up(), right? Yeah that would indeed be a bit more work.
On 13/04/2022 12:51, Joachim Wiberg wrote: > On Tue, Apr 12, 2022 at 21:27, Nikolay Aleksandrov <razor@blackwall.org> wrote: >> On 11/04/2022 16:38, Joachim Wiberg wrote: >>> @@ -526,6 +526,10 @@ void br_dev_setup(struct net_device *dev) >>> br->bridge_ageing_time = br->ageing_time = BR_DEFAULT_AGEING_TIME; >>> dev->max_mtu = ETH_MAX_MTU; >>> + br_opt_toggle(br, BROPT_UNICAST_FLOOD, 1); >> This one must be false by default. It changes current default behaviour. >> Unknown unicast is not currently passed up to the bridge if the port is >> not in promisc mode, this will change it. You'll have to make it consistent >> with promisc (e.g. one way would be for promisc always to enable unicast flood >> and it won't be possible to be disabled while promisc). > > Ouch, my bad! Will look into how to let this have as little impact as > possible. I like your semantics there, promisc should always win. > >>> + br_opt_toggle(br, BROPT_MCAST_FLOOD, 1); >>> + br_opt_toggle(br, BROPT_BCAST_FLOOD, 1); >> >> s/1/true/ for consistency > > Of course, thanks! > >>> @@ -118,7 +118,8 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb >>> /* by definition the broadcast is also a multicast address */ >>> if (is_broadcast_ether_addr(eth_hdr(skb)->h_dest)) { >>> pkt_type = BR_PKT_BROADCAST; >>> - local_rcv = true; >>> + if (br_opt_get(br, BROPT_BCAST_FLOOD)) >>> + local_rcv = true; >>> } else { >>> pkt_type = BR_PKT_MULTICAST; >>> if (br_multicast_rcv(&brmctx, &pmctx, vlan, skb, vid)) >>> @@ -161,12 +162,16 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb >>> } >>> mcast_hit = true; >>> } else { >>> - local_rcv = true; >>> - br->dev->stats.multicast++; >>> + if (br_opt_get(br, BROPT_MCAST_FLOOD)) { >>> + local_rcv = true; >>> + br->dev->stats.multicast++; >>> + } >>> } >>> break; >>> case BR_PKT_UNICAST: >>> dst = br_fdb_find_rcu(br, eth_hdr(skb)->h_dest, vid); >>> + if (!dst && br_opt_get(br, BROPT_UNICAST_FLOOD)) >>> + local_rcv = true; >>> break; >> >> This adds new tests for all fast paths for host traffic, especially >> the port - port communication which is the most critical one. Please >> at least move the unicast test to the "else" block of "if (dst)" >> later. > > OK, will fix! > >> The other tests can be moved to host only code too, but would require >> bigger changes. Please try to keep the impact on the fast-path at >> minimum. > > Interesting, you mean by speculatively setting local_rcv = true and > postpone the decsion to br_pass_frame_up(), right? Yeah that would > indeed be a bit more work. Yes, I was thinking maybe local_rcv can become an enum with an exact reason for the local_rcv, so if it's > 0 do the local_rcv and br_pass_frame_up() then can make a proper decision without passing all of the vars. I haven't tried it, so not sure if it's feasible. :)
On Wed, Apr 13, 2022 at 12:58, Nikolay Aleksandrov <razor@blackwall.org> wrote: > On 13/04/2022 12:51, Joachim Wiberg wrote: >> Interesting, you mean by speculatively setting local_rcv = true and >> postpone the decsion to br_pass_frame_up(), right? Yeah that would >> indeed be a bit more work. > Yes, I was thinking maybe local_rcv can become an enum with an exact reason for the > local_rcv, so if it's > 0 do the local_rcv and br_pass_frame_up() then > can make a proper decision without passing all of the vars. I haven't tried it, > so not sure if it's feasible. :) Ah, yeah that could definitely work. Thanks, I'll keep that in mind! :)
diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c index 8d6bab244c4a..0aa7d21ac82c 100644 --- a/net/bridge/br_device.c +++ b/net/bridge/br_device.c @@ -526,6 +526,10 @@ void br_dev_setup(struct net_device *dev) br->bridge_ageing_time = br->ageing_time = BR_DEFAULT_AGEING_TIME; dev->max_mtu = ETH_MAX_MTU; + br_opt_toggle(br, BROPT_UNICAST_FLOOD, 1); + br_opt_toggle(br, BROPT_MCAST_FLOOD, 1); + br_opt_toggle(br, BROPT_BCAST_FLOOD, 1); + br_netfilter_rtable_init(br); br_stp_timer_init(br); br_multicast_init(br); diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c index 196417859c4a..d439b876bdf5 100644 --- a/net/bridge/br_input.c +++ b/net/bridge/br_input.c @@ -118,7 +118,8 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb /* by definition the broadcast is also a multicast address */ if (is_broadcast_ether_addr(eth_hdr(skb)->h_dest)) { pkt_type = BR_PKT_BROADCAST; - local_rcv = true; + if (br_opt_get(br, BROPT_BCAST_FLOOD)) + local_rcv = true; } else { pkt_type = BR_PKT_MULTICAST; if (br_multicast_rcv(&brmctx, &pmctx, vlan, skb, vid)) @@ -161,12 +162,16 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb } mcast_hit = true; } else { - local_rcv = true; - br->dev->stats.multicast++; + if (br_opt_get(br, BROPT_MCAST_FLOOD)) { + local_rcv = true; + br->dev->stats.multicast++; + } } break; case BR_PKT_UNICAST: dst = br_fdb_find_rcu(br, eth_hdr(skb)->h_dest, vid); + if (!dst && br_opt_get(br, BROPT_UNICAST_FLOOD)) + local_rcv = true; break; default: break; diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h index 18ccc3d5d296..683bd0ee4c64 100644 --- a/net/bridge/br_private.h +++ b/net/bridge/br_private.h @@ -449,6 +449,9 @@ enum net_bridge_opts { BROPT_VLAN_BRIDGE_BINDING, BROPT_MCAST_VLAN_SNOOPING_ENABLED, BROPT_MST_ENABLED, + BROPT_UNICAST_FLOOD, + BROPT_MCAST_FLOOD, + BROPT_BCAST_FLOOD, }; struct net_bridge {
The bridge itself is also a port, but unfortunately it does not (yet) have a 'struct net_bridge_port'. However, in many cases we want to treat it as a proper port so concessions have been made, e.g., NULL port or host_joined attributes. This patch is an attempt to more of the same by adding support for controlling flooding of unknown broadcast/unicast/multicast to the bridge. Something we often also want to control in an offloaded switching fabric. Signed-off-by: Joachim Wiberg <troglobit@gmail.com> --- net/bridge/br_device.c | 4 ++++ net/bridge/br_input.c | 11 ++++++++--- net/bridge/br_private.h | 3 +++ 3 files changed, 15 insertions(+), 3 deletions(-)