| Message ID | 20220411133837.318876-9-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: > Unknown multicast, MAC/IPv4/IPv6, should always be flooded according to > the per-port mcast_flood setting, as well as to detected and configured > mcast_router ports. > > This patch drops the mrouters_only classifier of unknown IP multicast > and moves the flow handling from br_multicast_flood() to br_flood(). > This in turn means br_flood() must know about multicast router ports. > > Signed-off-by: Joachim Wiberg <troglobit@gmail.com> > --- > net/bridge/br_forward.c | 11 +++++++++++ > net/bridge/br_multicast.c | 6 +----- > 2 files changed, 12 insertions(+), 5 deletions(-) > If you'd like to flood unknown mcast traffic when a router is present please add a new option which defaults to the current state (disabled). > diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c > index 02bb620d3b8d..ab5b97a8c12e 100644 > --- a/net/bridge/br_forward.c > +++ b/net/bridge/br_forward.c > @@ -199,9 +199,15 @@ static struct net_bridge_port *maybe_deliver( > void br_flood(struct net_bridge *br, struct sk_buff *skb, > enum br_pkt_type pkt_type, bool local_rcv, bool local_orig) > { > + struct net_bridge_mcast *brmctx = &br->multicast_ctx; Note this breaks per-vlan mcast. You have to use the inferred mctx. > + struct net_bridge_port *rport = NULL; > struct net_bridge_port *prev = NULL; > + struct hlist_node *rp = NULL; > struct net_bridge_port *p; > > + if (pkt_type == BR_PKT_MULTICAST) > + rp = br_multicast_get_first_rport_node(brmctx, skb); > + > list_for_each_entry_rcu(p, &br->port_list, list) { > /* Do not flood unicast traffic to ports that turn it off, nor > * other traffic if flood off, except for traffic we originate > @@ -212,6 +218,11 @@ void br_flood(struct net_bridge *br, struct sk_buff *skb, > continue; > break; > case BR_PKT_MULTICAST: > + rport = br_multicast_rport_from_node_skb(rp, skb); > + if (rport == p) { > + rp = rcu_dereference(hlist_next_rcu(rp)); > + break; > + } > if (!(p->flags & BR_MCAST_FLOOD) && skb->dev != br->dev) > continue; > break; > diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c > index db4f2641d1cd..c57e3bbb00ad 100644 > --- a/net/bridge/br_multicast.c > +++ b/net/bridge/br_multicast.c > @@ -3643,9 +3643,7 @@ static int br_multicast_ipv4_rcv(struct net_bridge_mcast *brmctx, > err = ip_mc_check_igmp(skb); > > if (err == -ENOMSG) { > - if (!ipv4_is_local_multicast(ip_hdr(skb)->daddr)) { > - BR_INPUT_SKB_CB(skb)->mrouters_only = 1; > - } else if (pim_ipv4_all_pim_routers(ip_hdr(skb)->daddr)) { > + if (pim_ipv4_all_pim_routers(ip_hdr(skb)->daddr)) { > if (ip_hdr(skb)->protocol == IPPROTO_PIM) > br_multicast_pim(brmctx, pmctx, skb); > } else if (ipv4_is_all_snoopers(ip_hdr(skb)->daddr)) { > @@ -3712,8 +3710,6 @@ static int br_multicast_ipv6_rcv(struct net_bridge_mcast *brmctx, > err = ipv6_mc_check_mld(skb); > > if (err == -ENOMSG || err == -ENODATA) { > - if (!ipv6_addr_is_ll_all_nodes(&ipv6_hdr(skb)->daddr)) > - BR_INPUT_SKB_CB(skb)->mrouters_only = 1; > if (err == -ENODATA && > ipv6_addr_is_all_snoopers(&ipv6_hdr(skb)->daddr)) > br_ip6_multicast_mrd_rcv(brmctx, pmctx, skb);
Hi Nik, and thank you for taking the time to respond! On Tue, Apr 12, 2022 at 16:59, Nikolay Aleksandrov <razor@blackwall.org> wrote: > On 11/04/2022 16:38, Joachim Wiberg wrote: >> Unknown multicast, MAC/IPv4/IPv6, should always be flooded according to >> the per-port mcast_flood setting, as well as to detected and configured >> mcast_router ports. I realize I should've included a reference to RFC4541 here. Will add that in the non-RFC patch. >> This patch drops the mrouters_only classifier of unknown IP multicast >> and moves the flow handling from br_multicast_flood() to br_flood(). >> This in turn means br_flood() must know about multicast router ports. > If you'd like to flood unknown mcast traffic when a router is present please add > a new option which defaults to the current state (disabled). I don't think we have to add another option, because according to the snooping RFC[1], section 2.1.2 Data Forwarding Rules: "3) [..] If a switch receives an unregistered packet, it must forward that packet on all ports to which an IGMP[2] router is attached. A switch may default to forwarding unregistered packets on all ports. Switches that do not forward unregistered packets to all ports must include a configuration option to force the flooding of unregistered packets on specified ports. [..]" From this I'd like to argue that our current behavior in the bridge is wrong. To me it's clear that, since we have a confiugration option, we should forward unknown IP multicast to all MCAST_FLOOD ports (as well as the router ports). Also, and more critically, the current behavior of offloaded switches do forwarding like this already. So there is a discrepancy currently between how the bridge forwards unknown multicast and how any underlying switchcore does it. Sure, we'll break bridge behavior slightly by forwarding to more ports than previous (until the group becomes known/registered), but we'd be standards compliant, and the behavior can still be controlled per-port. [1]: https://www.rfc-editor.org/rfc/rfc4541.html#section-2.1.2 [2]: Section 3 goes on to explain how this is similar also for MLD >> diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c >> index 02bb620d3b8d..ab5b97a8c12e 100644 >> --- a/net/bridge/br_forward.c >> +++ b/net/bridge/br_forward.c >> @@ -199,9 +199,15 @@ static struct net_bridge_port *maybe_deliver( >> void br_flood(struct net_bridge *br, struct sk_buff *skb, >> enum br_pkt_type pkt_type, bool local_rcv, bool local_orig) >> { >> + struct net_bridge_mcast *brmctx = &br->multicast_ctx; > Note this breaks per-vlan mcast. You have to use the inferred mctx. Thank you, this was one of the things I was really unsure about since the introduction of per-VLAN support. I'll extend the prototype and include the brmctx from br_handle_frame_finish(). Thanks! Best regards /Joachim
On 12/04/2022 20:27, Joachim Wiberg wrote: > > Hi Nik, > > and thank you for taking the time to respond! > > On Tue, Apr 12, 2022 at 16:59, Nikolay Aleksandrov <razor@blackwall.org> wrote: >> On 11/04/2022 16:38, Joachim Wiberg wrote: >>> Unknown multicast, MAC/IPv4/IPv6, should always be flooded according to >>> the per-port mcast_flood setting, as well as to detected and configured >>> mcast_router ports. > > I realize I should've included a reference to RFC4541 here. Will add > that in the non-RFC patch. > >>> This patch drops the mrouters_only classifier of unknown IP multicast >>> and moves the flow handling from br_multicast_flood() to br_flood(). >>> This in turn means br_flood() must know about multicast router ports. >> If you'd like to flood unknown mcast traffic when a router is present please add >> a new option which defaults to the current state (disabled). > > I don't think we have to add another option, because according to the > snooping RFC[1], section 2.1.2 Data Forwarding Rules: > > "3) [..] If a switch receives an unregistered packet, it must forward > that packet on all ports to which an IGMP[2] router is attached. A > switch may default to forwarding unregistered packets on all ports. > Switches that do not forward unregistered packets to all ports must > include a configuration option to force the flooding of unregistered > packets on specified ports. [..]" > > From this I'd like to argue that our current behavior in the bridge is > wrong. To me it's clear that, since we have a confiugration option, we > should forward unknown IP multicast to all MCAST_FLOOD ports (as well as > the router ports). Definitely not wrong. In fact: "Switches that do not forward unregistered packets to all ports must include a configuration option to force the flooding of unregistered packets on specified ports. [..]" is already implemented because the admin can mark any port as a router and enable flooding to it. > > Also, and more critically, the current behavior of offloaded switches do > forwarding like this already. So there is a discrepancy currently > between how the bridge forwards unknown multicast and how any underlying > switchcore does it. > > Sure, we'll break bridge behavior slightly by forwarding to more ports > than previous (until the group becomes known/registered), but we'd be > standards compliant, and the behavior can still be controlled per-port. > > [1]: https://www.rfc-editor.org/rfc/rfc4541.html#section-2.1.2 > [2]: Section 3 goes on to explain how this is similar also for MLD > RFC4541 is only recommending, it's not a mandatory behaviour. This default has been placed for a very long time and a lot of users and tests take it into consideration. We cannot break such assumptions and start suddenly flooding packets, but we can leave it up to the admin or distribution/network software to configure it as default. >>> diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c >>> index 02bb620d3b8d..ab5b97a8c12e 100644 >>> --- a/net/bridge/br_forward.c >>> +++ b/net/bridge/br_forward.c >>> @@ -199,9 +199,15 @@ static struct net_bridge_port *maybe_deliver( >>> void br_flood(struct net_bridge *br, struct sk_buff *skb, >>> enum br_pkt_type pkt_type, bool local_rcv, bool local_orig) >>> { >>> + struct net_bridge_mcast *brmctx = &br->multicast_ctx; >> Note this breaks per-vlan mcast. You have to use the inferred mctx. > > Thank you, this was one of the things I was really unsure about since > the introduction of per-VLAN support. I'll extend the prototype and > include the brmctx from br_handle_frame_finish(). Thanks! > > Best regards > /Joachim
On Tue, Apr 12, 2022 at 20:37, Nikolay Aleksandrov <razor@blackwall.org> wrote: > On 12/04/2022 20:27, Joachim Wiberg wrote: >> [snip] >> From this I'd like to argue that our current behavior in the bridge is >> wrong. To me it's clear that, since we have a confiugration option, we >> should forward unknown IP multicast to all MCAST_FLOOD ports (as well as >> the router ports). > Definitely not wrong. In fact: > "Switches that do not forward unregistered packets to all ports must > include a configuration option to force the flooding of unregistered > packets on specified ports. [..]" > is already implemented because the admin can mark any port as a router and > enable flooding to it. Hmm, I understand your point (here and below), and won't drive this point further. Instead I'll pick up on what you said in your first reply ... (below, last) Btw, thank you for taking the time to reply and explain your standpoint, really helps my understanding of how we can develop the bridge further, without breaking userspace! :) >> [1]: https://www.rfc-editor.org/rfc/rfc4541.html#section-2.1.2 > RFC4541 is only recommending, it's not a mandatory behaviour. This > default has been placed for a very long time and a lot of users and > tests take it into consideration. Noted. > We cannot break such assumptions and start suddenly flooding packets, > but we can leave it up to the admin or distribution/network software > to configure it as default. So, if I add a bridge flag, default off as you mentioned out earlier, which changes the default behavior of MCAST_FLOOD, then you'd be OK with that? Something cheeky like this perhaps: if (!ipv4_is_local_multicast(ip_hdr(skb)->daddr)) BR_INPUT_SKB_CB(skb)->mrouters_only = !br_opt_get(br, BROPT_MCAST_FLOOD_RFC4541); Best regards /Joachim
On 13/04/2022 11:51, Joachim Wiberg wrote: > On Tue, Apr 12, 2022 at 20:37, Nikolay Aleksandrov <razor@blackwall.org> wrote: >> On 12/04/2022 20:27, Joachim Wiberg wrote: >>> [snip] >>> From this I'd like to argue that our current behavior in the bridge is >>> wrong. To me it's clear that, since we have a confiugration option, we >>> should forward unknown IP multicast to all MCAST_FLOOD ports (as well as >>> the router ports). >> Definitely not wrong. In fact: >> "Switches that do not forward unregistered packets to all ports must >> include a configuration option to force the flooding of unregistered >> packets on specified ports. [..]" >> is already implemented because the admin can mark any port as a router and >> enable flooding to it. > > Hmm, I understand your point (here and below), and won't drive this > point further. Instead I'll pick up on what you said in your first > reply ... (below, last) > > Btw, thank you for taking the time to reply and explain your standpoint, > really helps my understanding of how we can develop the bridge further, > without breaking userspace! :) > >>> [1]: https://www.rfc-editor.org/rfc/rfc4541.html#section-2.1.2 >> RFC4541 is only recommending, it's not a mandatory behaviour. This >> default has been placed for a very long time and a lot of users and >> tests take it into consideration. > > Noted. > >> We cannot break such assumptions and start suddenly flooding packets, >> but we can leave it up to the admin or distribution/network software >> to configure it as default. > > So, if I add a bridge flag, default off as you mentioned out earlier, > which changes the default behavior of MCAST_FLOOD, then you'd be OK with > that? Something cheeky like this perhaps: > > if (!ipv4_is_local_multicast(ip_hdr(skb)->daddr)) > BR_INPUT_SKB_CB(skb)->mrouters_only = !br_opt_get(br, BROPT_MCAST_FLOOD_RFC4541); Exactly! And that is exactly what I had in mind when I wrote it. :) Thanks, Nik > > > Best regards > /Joachim
On 13/04/2022 11:55, Nikolay Aleksandrov wrote: > On 13/04/2022 11:51, Joachim Wiberg wrote: >> On Tue, Apr 12, 2022 at 20:37, Nikolay Aleksandrov <razor@blackwall.org> wrote: >>> On 12/04/2022 20:27, Joachim Wiberg wrote: >>>> [snip] >>>> From this I'd like to argue that our current behavior in the bridge is >>>> wrong. To me it's clear that, since we have a confiugration option, we >>>> should forward unknown IP multicast to all MCAST_FLOOD ports (as well as >>>> the router ports). >>> Definitely not wrong. In fact: >>> "Switches that do not forward unregistered packets to all ports must >>> include a configuration option to force the flooding of unregistered >>> packets on specified ports. [..]" >>> is already implemented because the admin can mark any port as a router and >>> enable flooding to it. >> >> Hmm, I understand your point (here and below), and won't drive this >> point further. Instead I'll pick up on what you said in your first >> reply ... (below, last) >> >> Btw, thank you for taking the time to reply and explain your standpoint, >> really helps my understanding of how we can develop the bridge further, >> without breaking userspace! :) >> >>>> [1]: https://www.rfc-editor.org/rfc/rfc4541.html#section-2.1.2 >>> RFC4541 is only recommending, it's not a mandatory behaviour. This >>> default has been placed for a very long time and a lot of users and >>> tests take it into consideration. >> >> Noted. >> >>> We cannot break such assumptions and start suddenly flooding packets, >>> but we can leave it up to the admin or distribution/network software >>> to configure it as default. >> >> So, if I add a bridge flag, default off as you mentioned out earlier, >> which changes the default behavior of MCAST_FLOOD, then you'd be OK with >> that? Something cheeky like this perhaps: >> >> if (!ipv4_is_local_multicast(ip_hdr(skb)->daddr)) >> BR_INPUT_SKB_CB(skb)->mrouters_only = !br_opt_get(br, BROPT_MCAST_FLOOD_RFC4541); > > Exactly! And that is exactly what I had in mind when I wrote it. :) > Just please use a different option name that better suggests what it does. > Thanks, > Nik > >> >> >> Best regards >> /Joachim >
On Wed, Apr 13, 2022 at 12:00, Nikolay Aleksandrov <razor@blackwall.org> wrote: > On 13/04/2022 11:55, Nikolay Aleksandrov wrote: >> On 13/04/2022 11:51, Joachim Wiberg wrote: >>> So, if I add a bridge flag, default off as you mentioned out earlier, >>> which changes the default behavior of MCAST_FLOOD, then you'd be OK with >>> that? Something cheeky like this perhaps: >>> if (!ipv4_is_local_multicast(ip_hdr(skb)->daddr)) >>> BR_INPUT_SKB_CB(skb)->mrouters_only = !br_opt_get(br, BROPT_MCAST_FLOOD_RFC4541); >> Exactly! And that is exactly what I had in mind when I wrote it. :) Awesome, thank you! :) > Just please use a different option name that better suggests what it does. Heh, yeah spent a good while with my colleague (Tobias) thinking about how to name this one. I'll see what I can come up with, but whatever shows up in the next patch iteration will be very open for discussion. Cheers /Joachim
diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c index 02bb620d3b8d..ab5b97a8c12e 100644 --- a/net/bridge/br_forward.c +++ b/net/bridge/br_forward.c @@ -199,9 +199,15 @@ static struct net_bridge_port *maybe_deliver( void br_flood(struct net_bridge *br, struct sk_buff *skb, enum br_pkt_type pkt_type, bool local_rcv, bool local_orig) { + struct net_bridge_mcast *brmctx = &br->multicast_ctx; + struct net_bridge_port *rport = NULL; struct net_bridge_port *prev = NULL; + struct hlist_node *rp = NULL; struct net_bridge_port *p; + if (pkt_type == BR_PKT_MULTICAST) + rp = br_multicast_get_first_rport_node(brmctx, skb); + list_for_each_entry_rcu(p, &br->port_list, list) { /* Do not flood unicast traffic to ports that turn it off, nor * other traffic if flood off, except for traffic we originate @@ -212,6 +218,11 @@ void br_flood(struct net_bridge *br, struct sk_buff *skb, continue; break; case BR_PKT_MULTICAST: + rport = br_multicast_rport_from_node_skb(rp, skb); + if (rport == p) { + rp = rcu_dereference(hlist_next_rcu(rp)); + break; + } if (!(p->flags & BR_MCAST_FLOOD) && skb->dev != br->dev) continue; break; diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c index db4f2641d1cd..c57e3bbb00ad 100644 --- a/net/bridge/br_multicast.c +++ b/net/bridge/br_multicast.c @@ -3643,9 +3643,7 @@ static int br_multicast_ipv4_rcv(struct net_bridge_mcast *brmctx, err = ip_mc_check_igmp(skb); if (err == -ENOMSG) { - if (!ipv4_is_local_multicast(ip_hdr(skb)->daddr)) { - BR_INPUT_SKB_CB(skb)->mrouters_only = 1; - } else if (pim_ipv4_all_pim_routers(ip_hdr(skb)->daddr)) { + if (pim_ipv4_all_pim_routers(ip_hdr(skb)->daddr)) { if (ip_hdr(skb)->protocol == IPPROTO_PIM) br_multicast_pim(brmctx, pmctx, skb); } else if (ipv4_is_all_snoopers(ip_hdr(skb)->daddr)) { @@ -3712,8 +3710,6 @@ static int br_multicast_ipv6_rcv(struct net_bridge_mcast *brmctx, err = ipv6_mc_check_mld(skb); if (err == -ENOMSG || err == -ENODATA) { - if (!ipv6_addr_is_ll_all_nodes(&ipv6_hdr(skb)->daddr)) - BR_INPUT_SKB_CB(skb)->mrouters_only = 1; if (err == -ENODATA && ipv6_addr_is_all_snoopers(&ipv6_hdr(skb)->daddr)) br_ip6_multicast_mrd_rcv(brmctx, pmctx, skb);
Unknown multicast, MAC/IPv4/IPv6, should always be flooded according to the per-port mcast_flood setting, as well as to detected and configured mcast_router ports. This patch drops the mrouters_only classifier of unknown IP multicast and moves the flow handling from br_multicast_flood() to br_flood(). This in turn means br_flood() must know about multicast router ports. Signed-off-by: Joachim Wiberg <troglobit@gmail.com> --- net/bridge/br_forward.c | 11 +++++++++++ net/bridge/br_multicast.c | 6 +----- 2 files changed, 12 insertions(+), 5 deletions(-)