| Message ID | 20190218122107.10097-1-nikolay@cumulusnetworks.com |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | [RFC,v2] net: bridge: don't flood known multicast traffic when snooping is enabled | expand |
Hi Nik, On Mon, Feb 18, 2019 at 02:21:07PM +0200, Nikolay Aleksandrov wrote: > This is v2 of the RFC patch which aims to forward packets to known > mdsts' ports only (the no querier case). After v1 I've kept > the previous behaviour when it comes to unregistered traffic or when > a querier is present. All of this is of course only with snooping > enabled. So with this patch the following changes should occur: > - No querier: forward known mdst traffic to its registered ports, > no change about unknown mcast (flood) > - Querier present: no change > > The reason to do this is simple - we want to respect the user's mdb > configuration in both cases, that is if the user adds static mdb entries > manually then we should use that information about forwarding traffic. > > What do you think ? > > * Notes > Traffic that is currently marked as mrouters_only: > - IPv4: non-local mcast traffic, igmp reports > - IPv6: non-all-nodes-dst mcast traffic, mldv1 reports > > Simple use case: > $ echo 1 > /sys/class/net/bridge/bridge/multicast_snooping > $ bridge mdb add dev bridge port swp1 grp 239.0.0.1 > - without a querier currently traffic for 239.0.0.1 will still be flooded, > with this change it will be forwarded only to swp1 Looks good to me. > > Ido, I know this doesn't solve the issue you brought up, maybe we should > have a separate discussion about acting on querier changes in the switch > driver or alternative solutions (e.g. always-flood-unknown-mcast knob). > Perhaps the bridge can notify the drivers on querier state changes. I think we need to reflect querier state to drivers. That's the only way to sync the two data paths. We can flood different packet types using different tables. The IPv6 packet types are like in the Linux bridge: * IPv6 all-hosts (ff02::1) * IPv6 unregistered multicast Currently we treat both as broadcast, but in case querier is present, we can switch the second to use the multicast flood table where we only flood to mrouter ports. But I wonder if we can simplify it. I believe that the main reason we take querier state into account in the data path is because multicast is enabled by default on the bridge. Users that would otherwise explicitly enable it would probably make sure they also have a querier in the network. The workaround I did in commit 9d45deb04c59 ("mlxsw: spectrum: Treat IPv6 unregistered multicast as broadcast") was in response to blocked IPv6 neighbour solicitation packets sent to the solicited-node multicast address. I now see that Russel (Cc-ed) bumped into the same problem [1]. If we trap such packets at L2 (we need it for ND suppression anyway) and let the Linux bridge take care of flooding them correctly, will it fix the problem? I'm basically asking if after the proposed fix we will still have basic scenarios broken by not taking querier state into account. Thanks [1] https://patchwork.ozlabs.org/patch/1043694/ > > This patch is meant about discussing the best way to solve the issue, > it's not thoroughly tested, in case we settle about the details I'll run > more tests. > > Thanks, > > Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> > --- > net/bridge/br_device.c | 5 +++-- > net/bridge/br_input.c | 5 +++-- > 2 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c > index 013323b6dbe4..e8c01409a7e7 100644 > --- a/net/bridge/br_device.c > +++ b/net/bridge/br_device.c > @@ -96,8 +96,9 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev) > } > > mdst = br_mdb_get(br, skb, vid); > - if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) && > - br_multicast_querier_exists(br, eth_hdr(skb))) > + if (mdst || > + (BR_INPUT_SKB_CB_MROUTERS_ONLY(skb) && > + br_multicast_querier_exists(br, eth_hdr(skb)))) > br_multicast_flood(mdst, skb, false, true); > else > br_flood(br, skb, BR_PKT_MULTICAST, false, true); > diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c > index 5ea7e56119c1..8777566f7b6d 100644 > --- a/net/bridge/br_input.c > +++ b/net/bridge/br_input.c > @@ -136,8 +136,9 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb > switch (pkt_type) { > case BR_PKT_MULTICAST: > mdst = br_mdb_get(br, skb, vid); > - if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) && > - br_multicast_querier_exists(br, eth_hdr(skb))) { > + if (mdst || > + (BR_INPUT_SKB_CB_MROUTERS_ONLY(skb) && > + br_multicast_querier_exists(br, eth_hdr(skb)))) { > if ((mdst && mdst->host_joined) || > br_multicast_is_router(br)) { > local_rcv = true; > -- > 2.20.1 >
On Mon, Feb 18, 2019 at 02:21:07PM +0200, Nikolay Aleksandrov wrote: > This is v2 of the RFC patch which aims to forward packets to known > mdsts' ports only (the no querier case). After v1 I've kept > the previous behaviour when it comes to unregistered traffic or when > a querier is present. All of this is of course only with snooping > enabled. So with this patch the following changes should occur: > - No querier: forward known mdst traffic to its registered ports, > no change about unknown mcast (flood) > - Querier present: no change > > The reason to do this is simple - we want to respect the user's mdb > configuration in both cases, that is if the user adds static mdb entries > manually then we should use that information about forwarding traffic. > > What do you think ? > > * Notes > Traffic that is currently marked as mrouters_only: > - IPv4: non-local mcast traffic, igmp reports > - IPv6: non-all-nodes-dst mcast traffic, mldv1 reports > > Simple use case: > $ echo 1 > /sys/class/net/bridge/bridge/multicast_snooping > $ bridge mdb add dev bridge port swp1 grp 239.0.0.1 > - without a querier currently traffic for 239.0.0.1 will still be flooded, > with this change it will be forwarded only to swp1 There is still the issue with unsolicited reports adding mdst entries here, too. Leading to unwanted packet loss and connectivity issues. If I understand correctly, then the goal is to give the user slightly more control over specific, dedicated multicast addresses, right? Could you achieve the same via netfilter? Or is that rather unfeasible with switchdev drivers? (sorry, don't have much knowledge/experience with switchdev yet) Regards, Linus
On Tue, Feb 19, 2019 at 09:57:16AM +0100, Linus Lüssing wrote: > On Mon, Feb 18, 2019 at 02:21:07PM +0200, Nikolay Aleksandrov wrote: > > This is v2 of the RFC patch which aims to forward packets to known > > mdsts' ports only (the no querier case). After v1 I've kept > > the previous behaviour when it comes to unregistered traffic or when > > a querier is present. All of this is of course only with snooping > > enabled. So with this patch the following changes should occur: > > - No querier: forward known mdst traffic to its registered ports, > > no change about unknown mcast (flood) > > - Querier present: no change > > > > The reason to do this is simple - we want to respect the user's mdb > > configuration in both cases, that is if the user adds static mdb entries > > manually then we should use that information about forwarding traffic. > > > > What do you think ? > > > > * Notes > > Traffic that is currently marked as mrouters_only: > > - IPv4: non-local mcast traffic, igmp reports > > - IPv6: non-all-nodes-dst mcast traffic, mldv1 reports > > > > Simple use case: > > $ echo 1 > /sys/class/net/bridge/bridge/multicast_snooping > > $ bridge mdb add dev bridge port swp1 grp 239.0.0.1 > > - without a querier currently traffic for 239.0.0.1 will still be flooded, > > with this change it will be forwarded only to swp1 > > There is still the issue with unsolicited reports adding mdst > entries here, too. Leading to unwanted packet loss and connectivity issues. Or in other words, an unsolicited report will turn a previously unregistered multicast group into a registered one. However in the absence of a querier the knowledge about this newly registered multicast group will be incomplete. And therefore still needs to be flooded to avoid packet loss.
On 19/02/2019 11:21, Linus Lüssing wrote: > On Tue, Feb 19, 2019 at 09:57:16AM +0100, Linus Lüssing wrote: >> On Mon, Feb 18, 2019 at 02:21:07PM +0200, Nikolay Aleksandrov wrote: >>> This is v2 of the RFC patch which aims to forward packets to known >>> mdsts' ports only (the no querier case). After v1 I've kept >>> the previous behaviour when it comes to unregistered traffic or when >>> a querier is present. All of this is of course only with snooping >>> enabled. So with this patch the following changes should occur: >>> - No querier: forward known mdst traffic to its registered ports, >>> no change about unknown mcast (flood) >>> - Querier present: no change >>> >>> The reason to do this is simple - we want to respect the user's mdb >>> configuration in both cases, that is if the user adds static mdb entries >>> manually then we should use that information about forwarding traffic. >>> >>> What do you think ? >>> >>> * Notes >>> Traffic that is currently marked as mrouters_only: >>> - IPv4: non-local mcast traffic, igmp reports >>> - IPv6: non-all-nodes-dst mcast traffic, mldv1 reports >>> >>> Simple use case: >>> $ echo 1 > /sys/class/net/bridge/bridge/multicast_snooping >>> $ bridge mdb add dev bridge port swp1 grp 239.0.0.1 >>> - without a querier currently traffic for 239.0.0.1 will still be flooded, >>> with this change it will be forwarded only to swp1 >> >> There is still the issue with unsolicited reports adding mdst >> entries here, too. Leading to unwanted packet loss and connectivity issues. > > Or in other words, an unsolicited report will turn a previously > unregistered multicast group into a registered one. However in the > absence of a querier the knowledge about this newly registered multicast group > will be incomplete. And therefore still needs to be flooded to avoid packet > loss. > Right, this is expected. If the user has enabled igmp snooping and doesn't have a querier present then such behaviour is to be expected. What is surprising is the user explicitly enabling igmp snooping, adding an mdst and then still getting it flooded. :) An alternative is to drop all unregistered traffic when a querier is not present. But that will surely break setups and at best should be a configurable option that is disabled by default. So in effect and to try and make everybody happy we can add an option to control this behaviour with keeping the current as default and adding the following options: - no querier: flood all (default, current) - no querier: flood unregistered, forward registered - no querier: drop unregistered, forward registered
On Tue, Feb 19, 2019 at 03:31:42PM +0200, Nikolay Aleksandrov wrote: > On 19/02/2019 11:21, Linus Lüssing wrote: > > On Tue, Feb 19, 2019 at 09:57:16AM +0100, Linus Lüssing wrote: > >> On Mon, Feb 18, 2019 at 02:21:07PM +0200, Nikolay Aleksandrov wrote: > >>> This is v2 of the RFC patch which aims to forward packets to known > >>> mdsts' ports only (the no querier case). After v1 I've kept > >>> the previous behaviour when it comes to unregistered traffic or when > >>> a querier is present. All of this is of course only with snooping > >>> enabled. So with this patch the following changes should occur: > >>> - No querier: forward known mdst traffic to its registered ports, > >>> no change about unknown mcast (flood) > >>> - Querier present: no change > >>> > >>> The reason to do this is simple - we want to respect the user's mdb > >>> configuration in both cases, that is if the user adds static mdb entries > >>> manually then we should use that information about forwarding traffic. > >>> > >>> What do you think ? > >>> > >>> * Notes > >>> Traffic that is currently marked as mrouters_only: > >>> - IPv4: non-local mcast traffic, igmp reports > >>> - IPv6: non-all-nodes-dst mcast traffic, mldv1 reports > >>> > >>> Simple use case: > >>> $ echo 1 > /sys/class/net/bridge/bridge/multicast_snooping > >>> $ bridge mdb add dev bridge port swp1 grp 239.0.0.1 > >>> - without a querier currently traffic for 239.0.0.1 will still be flooded, > >>> with this change it will be forwarded only to swp1 > >> > >> There is still the issue with unsolicited reports adding mdst > >> entries here, too. Leading to unwanted packet loss and connectivity issues. > > > > Or in other words, an unsolicited report will turn a previously > > unregistered multicast group into a registered one. However in the > > absence of a querier the knowledge about this newly registered multicast group > > will be incomplete. And therefore still needs to be flooded to avoid packet > > loss. > > > > Right, this is expected. If the user has enabled igmp snooping and doesn't have > a querier present then such behaviour is to be expected. IGMP snooping is currently enabled by default. And IGMP/MLD querier is disabled by default. I wouldn't want packet loss to be the expected behaviour in a default setup. This default setup currently works. However with this change it will introduce packet loss, as you acknowledged (if I understood you correctly?). > What is surprising is > the user explicitly enabling igmp snooping, adding an mdst and then still > getting it flooded. :) Hm, for me that does not seem surprising. I would not expect igmp snooping to work without a querier on the link. Why don't you just add/enable a querier in your setups then if you want to avoid flooding? > An alternative is to drop all unregistered traffic when a querier is not present. > But that will surely break setups and at best should be a configurable option that > is disabled by default. Absolutely right. Always dropping with no querier is no option. That's why I'd say you should always flood multicast packets if there is no querier. > So in effect and to try and make everybody happy we can add an option to control > this behaviour with keeping the current as default and adding the following options: > - no querier: flood all (default, current) ACK For the other options maybe I do not understand your scenario yet. Wouldn't these two options result in eratic behaviour? > - no querier: flood unregistered, forward registered > - no querier: drop unregistered, forward registered Let's call these two cases A) - flood unregistered, forward registered and B) - drop unregistered, forward registered. Let's say you have a bridge with two ports: (1)<-[br]->(2). And no querier. Behind (2) there is a listener for group M. M is not registered by the bridge because either that listener joined before the bridge came up or the entry was registered once but had timed out. Or there was packet loss and the report did not arrive at the bridge (for instance bc. listener is behind a wireless connection). For case B) we can immediately see that the listener at (2) will not receive the traffic it signed up for. And this is a permanent issue as the listener will not send any further reports. Case A) is ok, the listener behind port (2) receives its traffic. Now, a listener for M joins at (1). It sends an unsolicited report. Group M becomes registered by the bridge. Both for cases (A) and (B) this new listener at (1) will receive its traffic. However, not only in case B) now, but in case A), too, the listener at (2) will rceive no more traffic for M. Now 260 seconds pass (multicast_membership_interval). The entry for M times out and is deleted on the bridge. It becomes unregistered. Now for case (A) things would be ok again, both listeners at (1) and (2) would receive traffic. For now - as long as no new listener joins again. For case (B), both the listener at port (1) and (2) will fail to receive the traffic they signed up for. --- I hope this illustrates a bit what I'm afraid of? If you have any measures to prevent such behavior in your setup, I'd be curious to know. Regards, Linus
On 19/02/2019 17:42, Linus Lüssing wrote: > On Tue, Feb 19, 2019 at 03:31:42PM +0200, Nikolay Aleksandrov wrote: >> On 19/02/2019 11:21, Linus Lüssing wrote: >>> On Tue, Feb 19, 2019 at 09:57:16AM +0100, Linus Lüssing wrote: >>>> On Mon, Feb 18, 2019 at 02:21:07PM +0200, Nikolay Aleksandrov wrote: >>>>> This is v2 of the RFC patch which aims to forward packets to known >>>>> mdsts' ports only (the no querier case). After v1 I've kept >>>>> the previous behaviour when it comes to unregistered traffic or when >>>>> a querier is present. All of this is of course only with snooping >>>>> enabled. So with this patch the following changes should occur: >>>>> - No querier: forward known mdst traffic to its registered ports, >>>>> no change about unknown mcast (flood) >>>>> - Querier present: no change >>>>> >>>>> The reason to do this is simple - we want to respect the user's mdb >>>>> configuration in both cases, that is if the user adds static mdb entries >>>>> manually then we should use that information about forwarding traffic. >>>>> >>>>> What do you think ? >>>>> >>>>> * Notes >>>>> Traffic that is currently marked as mrouters_only: >>>>> - IPv4: non-local mcast traffic, igmp reports >>>>> - IPv6: non-all-nodes-dst mcast traffic, mldv1 reports >>>>> >>>>> Simple use case: >>>>> $ echo 1 > /sys/class/net/bridge/bridge/multicast_snooping >>>>> $ bridge mdb add dev bridge port swp1 grp 239.0.0.1 >>>>> - without a querier currently traffic for 239.0.0.1 will still be flooded, >>>>> with this change it will be forwarded only to swp1 >>>> >>>> There is still the issue with unsolicited reports adding mdst >>>> entries here, too. Leading to unwanted packet loss and connectivity issues. >>> >>> Or in other words, an unsolicited report will turn a previously >>> unregistered multicast group into a registered one. However in the >>> absence of a querier the knowledge about this newly registered multicast group >>> will be incomplete. And therefore still needs to be flooded to avoid packet >>> loss. >>> >> >> Right, this is expected. If the user has enabled igmp snooping and doesn't have >> a querier present then such behaviour is to be expected. > > IGMP snooping is currently enabled by default. And IGMP/MLD > querier is disabled by default. I wouldn't want packet loss to be > the expected behaviour in a default setup. > > This default setup currently works. However with this change it > will introduce packet loss, as you acknowledged (if I understood > you correctly?). > Yeah, I'm not pushing this change anymore, the only option was to add a tunable for the behaviour which I described in my previous reply but I really want to avoid adding yet another bridge option as it has way too many already. >> What is surprising is >> the user explicitly enabling igmp snooping, adding an mdst and then still >> getting it flooded. :) > > Hm, for me that does not seem surprising. I would not expect igmp > snooping to work without a querier on the link. Why don't you just > add/enable a querier in your setups then if you want to avoid > flooding? > Yep, ack. > >> An alternative is to drop all unregistered traffic when a querier is not present. >> But that will surely break setups and at best should be a configurable option that >> is disabled by default. > > Absolutely right. Always dropping with no querier is no option. That's why I'd say > you should always flood multicast packets if there is no querier. > > >> So in effect and to try and make everybody happy we can add an option to control >> this behaviour with keeping the current as default and adding the following options: >> - no querier: flood all (default, current) > > ACK > > For the other options maybe I do not understand your scenario yet. > Wouldn't these two options result in eratic behaviour? > >> - no querier: flood unregistered, forward registered >> - no querier: drop unregistered, forward registered > To be fair I've seen all 3 options being default on different versions of network OSes by major vendors. > Let's call these two cases A) - flood unregistered, forward > registered and B) - drop unregistered, forward registered. > > > Let's say you have a bridge with two ports: > (1)<-[br]->(2). And no querier. > > Behind (2) there is a listener for group M. M is not > registered by the bridge because either that listener joined > before the bridge came up or the entry was registered once but had > timed out. Or there was packet loss and the report did not arrive > at the bridge (for instance bc. listener is behind a wireless > connection). > > For case B) we can immediately see that the listener at (2) will > not receive the traffic it signed up for. And this is a permanent > issue as the listener will not send any further reports. > > Case A) is ok, the listener behind port (2) receives its traffic. > > > Now, a listener for M joins at (1). It sends an unsolicited > report. Group M becomes registered by the bridge. Both for > cases (A) and (B) this new listener at (1) will receive its > traffic. However, not only in case B) now, but in case A), too, > the listener at (2) will rceive no more traffic for M. > > > Now 260 seconds pass (multicast_membership_interval). The entry > for M times out and is deleted on the bridge. It becomes > unregistered. > > Now for case (A) things would be ok again, both listeners at (1) > and (2) would receive traffic. For now - as long as no new listener > joins again. > > For case (B), both the listener at port (1) and (2) will fail to > receive the traffic they signed up for. > > > --- > > I hope this illustrates a bit what I'm afraid of? If you have any > measures to prevent such behavior in your setup, I'd be curious to > know. > It does and I'm aware of these scenarios, but there're static mcast trees and also such that are controlled by user-space daemons, that is the daemon keeps refreshing the mdb entries. They're non-standard for sure and obviously we suggest having a querier always. That's why I suggested having these as options, but now I'm retracting that and will table this whole thing for the time being. I really don't want to add another option to the bridge lightly. > Regards, Linus > Thanks for the discussion and all the helpful comments, Nik
diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c index 013323b6dbe4..e8c01409a7e7 100644 --- a/net/bridge/br_device.c +++ b/net/bridge/br_device.c @@ -96,8 +96,9 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev) } mdst = br_mdb_get(br, skb, vid); - if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) && - br_multicast_querier_exists(br, eth_hdr(skb))) + if (mdst || + (BR_INPUT_SKB_CB_MROUTERS_ONLY(skb) && + br_multicast_querier_exists(br, eth_hdr(skb)))) br_multicast_flood(mdst, skb, false, true); else br_flood(br, skb, BR_PKT_MULTICAST, false, true); diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c index 5ea7e56119c1..8777566f7b6d 100644 --- a/net/bridge/br_input.c +++ b/net/bridge/br_input.c @@ -136,8 +136,9 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb switch (pkt_type) { case BR_PKT_MULTICAST: mdst = br_mdb_get(br, skb, vid); - if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) && - br_multicast_querier_exists(br, eth_hdr(skb))) { + if (mdst || + (BR_INPUT_SKB_CB_MROUTERS_ONLY(skb) && + br_multicast_querier_exists(br, eth_hdr(skb)))) { if ((mdst && mdst->host_joined) || br_multicast_is_router(br)) { local_rcv = true;
This is v2 of the RFC patch which aims to forward packets to known mdsts' ports only (the no querier case). After v1 I've kept the previous behaviour when it comes to unregistered traffic or when a querier is present. All of this is of course only with snooping enabled. So with this patch the following changes should occur: - No querier: forward known mdst traffic to its registered ports, no change about unknown mcast (flood) - Querier present: no change The reason to do this is simple - we want to respect the user's mdb configuration in both cases, that is if the user adds static mdb entries manually then we should use that information about forwarding traffic. What do you think ? * Notes Traffic that is currently marked as mrouters_only: - IPv4: non-local mcast traffic, igmp reports - IPv6: non-all-nodes-dst mcast traffic, mldv1 reports Simple use case: $ echo 1 > /sys/class/net/bridge/bridge/multicast_snooping $ bridge mdb add dev bridge port swp1 grp 239.0.0.1 - without a querier currently traffic for 239.0.0.1 will still be flooded, with this change it will be forwarded only to swp1 Ido, I know this doesn't solve the issue you brought up, maybe we should have a separate discussion about acting on querier changes in the switch driver or alternative solutions (e.g. always-flood-unknown-mcast knob). Perhaps the bridge can notify the drivers on querier state changes. This patch is meant about discussing the best way to solve the issue, it's not thoroughly tested, in case we settle about the details I'll run more tests. Thanks, Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> --- net/bridge/br_device.c | 5 +++-- net/bridge/br_input.c | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-)