[RFC net-next] net: dsa: add support for MC_DISABLED attribute

[RFC net-next] net: dsa: add support for MC_DISABLED attribute

[Vivien Didelot]

This patch adds support for enabling or disabling the flooding of
unknown multicast traffic on the CPU ports, depending on the value
of the switchdev SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED attribute.

This allows the user to prevent the CPU to be flooded with a lot of
undesirable traffic that the network stack needs to filter in software.

The bridge has multicast snooping enabled by default, hence CPU ports
aren't bottlenecked with arbitrary network applications anymore.
But this can be an issue in some scenarios such as pinging the bridge's
IPv6 address. Setting /sys/class/net/br0/bridge/multicast_snooping to
0 would restore unknown multicast flooding and thus fix ICMPv6. As
an alternative, enabling multicast_querier would program the bridge
address into the switch.

Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
---
 net/dsa/dsa_priv.h |  2 ++
 net/dsa/port.c     | 19 +++++++++++++++++++
 net/dsa/slave.c    |  4 ++++
 3 files changed, 25 insertions(+)

diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index a4853c22c2ff..54d838956499 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -163,6 +163,8 @@ int dsa_port_pre_bridge_flags(const struct dsa_port *dp, unsigned long flags,
 			      struct switchdev_trans *trans);
 int dsa_port_bridge_flags(const struct dsa_port *dp, unsigned long flags,
 			  struct switchdev_trans *trans);
+int dsa_port_bridge_mc_disabled(const struct dsa_port *dp, bool mc_disabled,
+				struct switchdev_trans *trans);
 int dsa_port_vlan_add(struct dsa_port *dp,
 		      const struct switchdev_obj_port_vlan *vlan,
 		      struct switchdev_trans *trans);
diff --git a/net/dsa/port.c b/net/dsa/port.c
index d2b65e8dc60c..79d14c36ef9a 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -261,6 +261,25 @@ int dsa_port_bridge_flags(const struct dsa_port *dp, unsigned long flags,
 	return err;
 }
 
+int dsa_port_bridge_mc_disabled(const struct dsa_port *dp, bool mc_disabled,
+				struct switchdev_trans *trans)
+{
+	struct dsa_switch *ds = dp->ds;
+	int port = dp->index;
+
+	if (switchdev_trans_ph_prepare(trans)) {
+		if (!ds->ops->port_egress_floods)
+			return -EOPNOTSUPP;
+
+		return 0;
+	}
+
+	/* When multicast snooping is disabled,
+	 * every multicast packet should be flooded to the CPU port.
+         */
+	return ds->ops->port_egress_floods(ds, port, true, mc_disabled);
+}
+
 int dsa_port_fdb_add(struct dsa_port *dp, const unsigned char *addr,
 		     u16 vid)
 {
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index db58e748557d..9308ffa4f22c 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -302,6 +302,10 @@ static int dsa_slave_port_attr_set(struct net_device *dev,
 	case SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS:
 		ret = dsa_port_bridge_flags(dp, attr->u.brport_flags, trans);
 		break;
+	case SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED:
+		ret = dsa_port_bridge_mc_disabled(dp->cpu_dp,
+						  attr->u.mc_disabled, trans);
+		break;
 	default:
 		ret = -EOPNOTSUPP;
 		break;
-- 
2.22.0

Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute

[Florian Fainelli]

On 6/20/2019 4:56 PM, Vivien Didelot wrote:
> This patch adds support for enabling or disabling the flooding of
> unknown multicast traffic on the CPU ports, depending on the value
> of the switchdev SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED attribute.
> 
> This allows the user to prevent the CPU to be flooded with a lot of
> undesirable traffic that the network stack needs to filter in software.
> 
> The bridge has multicast snooping enabled by default, hence CPU ports
> aren't bottlenecked with arbitrary network applications anymore.
> But this can be an issue in some scenarios such as pinging the bridge's
> IPv6 address. Setting /sys/class/net/br0/bridge/multicast_snooping to
> 0 would restore unknown multicast flooding and thus fix ICMPv6. As
> an alternative, enabling multicast_querier would program the bridge
> address into the switch.
From what I can read from mlxsw, we should probably also implement the
SWITCHDEV_ATTR_ID_PORT_MROUTER attribute in order to be consistent.

Since the attribute MC_DISABLED is on the bridge master, we should also
iterate over the list of switch ports being a member of that bridge and
change their flooding attribute, taking into account whether
BR_MCAST_FLOOD is set on that particular port or not. Just paraphrasing
what mlxsw does here again...

Once you act on the user-facing ports, you might be able to leave the
CPU port flooding unconditionally, since it would only "flood" the CPU
port either because an user-facing port has BR_MCAST_FLOOD set, or
because this is known MC traffic that got programmed via the bridge's
MDB. Would that work?

On a higher level, I really wish we did not have to re-implement a lot
of identical or similar logic in each switch drivers and had a more
central model of what is behaviorally expected.

[snip]

> +int dsa_port_bridge_mc_disabled(const struct dsa_port *dp, bool mc_disabled,
> +				struct switchdev_trans *trans)
> +{
> +	struct dsa_switch *ds = dp->ds;
> +	int port = dp->index;
> +
> +	if (switchdev_trans_ph_prepare(trans)) {
> +		if (!ds->ops->port_egress_floods)
> +			return -EOPNOTSUPP;
> +
> +		return 0;
> +	}
> +
> +	/* When multicast snooping is disabled,
> +	 * every multicast packet should be flooded to the CPU port.
> +         */

The comment alignment is a bit off.
-- 
Florian

Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute

[Vivien Didelot]

On Thu, 20 Jun 2019 19:24:47 -0700, Florian Fainelli <f.fainelli@gmail.com> wrote:
> > This patch adds support for enabling or disabling the flooding of
> > unknown multicast traffic on the CPU ports, depending on the value
> > of the switchdev SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED attribute.
> > 
> > This allows the user to prevent the CPU to be flooded with a lot of
> > undesirable traffic that the network stack needs to filter in software.
> > 
> > The bridge has multicast snooping enabled by default, hence CPU ports
> > aren't bottlenecked with arbitrary network applications anymore.
> > But this can be an issue in some scenarios such as pinging the bridge's
> > IPv6 address. Setting /sys/class/net/br0/bridge/multicast_snooping to
> > 0 would restore unknown multicast flooding and thus fix ICMPv6. As
> > an alternative, enabling multicast_querier would program the bridge
> > address into the switch.
> From what I can read from mlxsw, we should probably also implement the
> SWITCHDEV_ATTR_ID_PORT_MROUTER attribute in order to be consistent.
> 
> Since the attribute MC_DISABLED is on the bridge master, we should also
> iterate over the list of switch ports being a member of that bridge and
> change their flooding attribute, taking into account whether
> BR_MCAST_FLOOD is set on that particular port or not. Just paraphrasing
> what mlxsw does here again...

Ouch, doesn't sound like what a driver should be doing :-(

Ido, I cannot find documentation for multicast_snooping or MC_DISABLED
and the naming isn't clear. Can this be considered as an equivalent
of mcast_flood but targeting the bridge device itself, describing
whether the bridge is interested or not in unknown multicast traffic?

> Once you act on the user-facing ports, you might be able to leave the
> CPU port flooding unconditionally, since it would only "flood" the CPU
> port either because an user-facing port has BR_MCAST_FLOOD set, or
> because this is known MC traffic that got programmed via the bridge's
> MDB. Would that work?

You may want the machine or network connected behind a bridge port
to be flooded with unknown multicast traffic, without having the
CPU conduit clogged up with this traffic. So these are two distinct
settings for me.

The only scenario I can think of needing the CPU to be flooded is if
there's a non-DSA port in the bridge maybe. But IMHO this should be
handled by the bridge, offloading or not the appropriate attribute.

> On a higher level, I really wish we did not have to re-implement a lot
> of identical or similar logic in each switch drivers and had a more
> central model of what is behaviorally expected.

I couldn't agree more, ethernet switch drivers should only offload
the notified bridge configuration, not noodling their own logic...


Russell, Ido, Florian, so far I understand that a multicast-unaware
bridge must flood unknown traffic everywhere (CPU included);
and a multicast-aware bridge must only flood its ports if their
mcast_flood is on, and known traffic targeting the bridge must be
offloaded accordingly. Do you guys agree with this?


Thanks,
Vivien

Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute

[Russell King - ARM Linux admin]

On Fri, Jun 21, 2019 at 05:29:52PM -0400, Vivien Didelot wrote:
> Russell, Ido, Florian, so far I understand that a multicast-unaware
> bridge must flood unknown traffic everywhere (CPU included);
                           ^
			multicast

> and a multicast-aware bridge must only flood its ports if their
                                              ^
				unknown multicast traffic to

> mcast_flood is on, and known traffic targeting the bridge must be
> offloaded accordingly. Do you guys agree with this?

I don't see a problem with that with the clarification that we're
talking about multicast packets here.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute

[Ido Schimmel]

On Thu, Jun 20, 2019 at 07:24:47PM -0700, Florian Fainelli wrote:
> On 6/20/2019 4:56 PM, Vivien Didelot wrote:
> > This patch adds support for enabling or disabling the flooding of
> > unknown multicast traffic on the CPU ports, depending on the value
> > of the switchdev SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED attribute.
> > 
> > This allows the user to prevent the CPU to be flooded with a lot of
> > undesirable traffic that the network stack needs to filter in software.
> > 
> > The bridge has multicast snooping enabled by default, hence CPU ports
> > aren't bottlenecked with arbitrary network applications anymore.
> > But this can be an issue in some scenarios such as pinging the bridge's
> > IPv6 address. Setting /sys/class/net/br0/bridge/multicast_snooping to
> > 0 would restore unknown multicast flooding and thus fix ICMPv6. As
> > an alternative, enabling multicast_querier would program the bridge
> > address into the switch.
> From what I can read from mlxsw, we should probably also implement the
> SWITCHDEV_ATTR_ID_PORT_MROUTER attribute in order to be consistent.
> 
> Since the attribute MC_DISABLED is on the bridge master, we should also
> iterate over the list of switch ports being a member of that bridge and
> change their flooding attribute, taking into account whether
> BR_MCAST_FLOOD is set on that particular port or not. Just paraphrasing
> what mlxsw does here again...

When multicast snooping is enabled, unregistered multicast traffic
should be flooded to mrouter ports only. Otherwise, it should be flooded
to all ports.

> Once you act on the user-facing ports, you might be able to leave the
> CPU port flooding unconditionally, since it would only "flood" the CPU
> port either because an user-facing port has BR_MCAST_FLOOD set, or
> because this is known MC traffic that got programmed via the bridge's
> MDB. Would that work?
> 
> On a higher level, I really wish we did not have to re-implement a lot
> of identical or similar logic in each switch drivers and had a more
> central model of what is behaviorally expected.

Well, that model is the bridge driver... But I agree that we can
probably simplify the interface towards drivers and move more code up
the stack.

For example, two things mlxsw is doing when multicast snooping is
enabled:

1. Writing MDB entries to the device. When multicast snooping is
disabled, MDB entries are ignored by the bridge driver. Can we agree to
have the bridge driver generate SWITCHDEV_OBJ_ID_PORT_MDB add / delete
for all MDB entries when multicast snooping is toggled?

2. Flooding unregistered multicast traffic only to mrouter ports. The
bridge driver can iterate over the bridge members and toggle
BR_MCAST_FLOOD accordingly. It will not actually change this value. Only
emulate this change towards drivers.

I will try to come up with a more detailed list later this week. I
really want to start moving more logic out of drivers.

Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute

[Ido Schimmel]

On Fri, Jun 21, 2019 at 05:29:52PM -0400, Vivien Didelot wrote:
> On Thu, 20 Jun 2019 19:24:47 -0700, Florian Fainelli <f.fainelli@gmail.com> wrote:
> > > This patch adds support for enabling or disabling the flooding of
> > > unknown multicast traffic on the CPU ports, depending on the value
> > > of the switchdev SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED attribute.
> > > 
> > > This allows the user to prevent the CPU to be flooded with a lot of
> > > undesirable traffic that the network stack needs to filter in software.
> > > 
> > > The bridge has multicast snooping enabled by default, hence CPU ports
> > > aren't bottlenecked with arbitrary network applications anymore.
> > > But this can be an issue in some scenarios such as pinging the bridge's
> > > IPv6 address. Setting /sys/class/net/br0/bridge/multicast_snooping to
> > > 0 would restore unknown multicast flooding and thus fix ICMPv6. As
> > > an alternative, enabling multicast_querier would program the bridge
> > > address into the switch.
> > From what I can read from mlxsw, we should probably also implement the
> > SWITCHDEV_ATTR_ID_PORT_MROUTER attribute in order to be consistent.
> > 
> > Since the attribute MC_DISABLED is on the bridge master, we should also
> > iterate over the list of switch ports being a member of that bridge and
> > change their flooding attribute, taking into account whether
> > BR_MCAST_FLOOD is set on that particular port or not. Just paraphrasing
> > what mlxsw does here again...
> 
> Ouch, doesn't sound like what a driver should be doing :-(
> 
> Ido, I cannot find documentation for multicast_snooping or MC_DISABLED
> and the naming isn't clear. Can this be considered as an equivalent
> of mcast_flood but targeting the bridge device itself, describing
> whether the bridge is interested or not in unknown multicast traffic?

Not really, you're only looking at one aspect of this, but there is
more. For example, when multicast snooping is disabled, the MDB is not
considered.

> > Once you act on the user-facing ports, you might be able to leave the
> > CPU port flooding unconditionally, since it would only "flood" the CPU
> > port either because an user-facing port has BR_MCAST_FLOOD set, or
> > because this is known MC traffic that got programmed via the bridge's
> > MDB. Would that work?
> 
> You may want the machine or network connected behind a bridge port
> to be flooded with unknown multicast traffic, without having the
> CPU conduit clogged up with this traffic. So these are two distinct
> settings for me.
> 
> The only scenario I can think of needing the CPU to be flooded is if
> there's a non-DSA port in the bridge maybe. But IMHO this should be
> handled by the bridge, offloading or not the appropriate attribute.
> 
> > On a higher level, I really wish we did not have to re-implement a lot
> > of identical or similar logic in each switch drivers and had a more
> > central model of what is behaviorally expected.
> 
> I couldn't agree more, ethernet switch drivers should only offload
> the notified bridge configuration, not noodling their own logic...

Please see my comment to Florian. The driver is doing what needs to be
done in order to comply with the behavior of the bridge driver. As I
wrote to Florian, we can probably move some logic up the stack and
simplify drivers.

> Russell, Ido, Florian, so far I understand that a multicast-unaware
> bridge must flood unknown traffic everywhere (CPU included);
> and a multicast-aware bridge must only flood its ports if their
> mcast_flood is on, and known traffic targeting the bridge must be
> offloaded accordingly. Do you guys agree with this?

When multicast snooping is enabled unregistered multicast traffic should
only be flooded to mrouter ports.

Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute

[Russell King - ARM Linux admin]

On Sun, Jun 23, 2019 at 07:09:52AM +0000, Ido Schimmel wrote:
> When multicast snooping is enabled unregistered multicast traffic should
> only be flooded to mrouter ports.

Given that IPv6 relies upon multicast working, and multicast snooping
is a kernel configuration option, and MLD messages will only be sent
when whenever the configuration on the target changes, and there may
not be a multicast querier in the system, who does that ensure that
IPv6 can work on a bridge where the kernel configured and built with
multicast snooping enabled?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute

[Ido Schimmel]

On Sun, Jun 23, 2019 at 08:26:05AM +0100, Russell King - ARM Linux admin wrote:
> On Sun, Jun 23, 2019 at 07:09:52AM +0000, Ido Schimmel wrote:
> > When multicast snooping is enabled unregistered multicast traffic should
> > only be flooded to mrouter ports.
> 
> Given that IPv6 relies upon multicast working, and multicast snooping
> is a kernel configuration option, and MLD messages will only be sent
> when whenever the configuration on the target changes, and there may
> not be a multicast querier in the system, who does that ensure that
> IPv6 can work on a bridge where the kernel configured and built with
> multicast snooping enabled?

See commit b00589af3b04 ("bridge: disable snooping if there is no
querier"). I think that's unfortunate behavior that we need because
multicast snooping is enabled by default. If it weren't enabled by
default, then anyone enabling it would also make sure there's a querier
in the network.

Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute

[Ido Schimmel]

On Sun, Jun 23, 2019 at 06:48:41AM +0000, Ido Schimmel wrote:
> On Thu, Jun 20, 2019 at 07:24:47PM -0700, Florian Fainelli wrote:
> > On 6/20/2019 4:56 PM, Vivien Didelot wrote:
> > > This patch adds support for enabling or disabling the flooding of
> > > unknown multicast traffic on the CPU ports, depending on the value
> > > of the switchdev SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED attribute.
> > > 
> > > This allows the user to prevent the CPU to be flooded with a lot of
> > > undesirable traffic that the network stack needs to filter in software.
> > > 
> > > The bridge has multicast snooping enabled by default, hence CPU ports
> > > aren't bottlenecked with arbitrary network applications anymore.
> > > But this can be an issue in some scenarios such as pinging the bridge's
> > > IPv6 address. Setting /sys/class/net/br0/bridge/multicast_snooping to
> > > 0 would restore unknown multicast flooding and thus fix ICMPv6. As
> > > an alternative, enabling multicast_querier would program the bridge
> > > address into the switch.
> > From what I can read from mlxsw, we should probably also implement the
> > SWITCHDEV_ATTR_ID_PORT_MROUTER attribute in order to be consistent.
> > 
> > Since the attribute MC_DISABLED is on the bridge master, we should also
> > iterate over the list of switch ports being a member of that bridge and
> > change their flooding attribute, taking into account whether
> > BR_MCAST_FLOOD is set on that particular port or not. Just paraphrasing
> > what mlxsw does here again...
> 
> When multicast snooping is enabled, unregistered multicast traffic
> should be flooded to mrouter ports only. Otherwise, it should be flooded
> to all ports.
> 
> > Once you act on the user-facing ports, you might be able to leave the
> > CPU port flooding unconditionally, since it would only "flood" the CPU
> > port either because an user-facing port has BR_MCAST_FLOOD set, or
> > because this is known MC traffic that got programmed via the bridge's
> > MDB. Would that work?
> > 
> > On a higher level, I really wish we did not have to re-implement a lot
> > of identical or similar logic in each switch drivers and had a more
> > central model of what is behaviorally expected.
> 
> Well, that model is the bridge driver... But I agree that we can
> probably simplify the interface towards drivers and move more code up
> the stack.
> 
> For example, two things mlxsw is doing when multicast snooping is
> enabled:
> 
> 1. Writing MDB entries to the device. When multicast snooping is
> disabled, MDB entries are ignored by the bridge driver. Can we agree to
> have the bridge driver generate SWITCHDEV_OBJ_ID_PORT_MDB add / delete
> for all MDB entries when multicast snooping is toggled?
> 
> 2. Flooding unregistered multicast traffic only to mrouter ports. The
> bridge driver can iterate over the bridge members and toggle
> BR_MCAST_FLOOD accordingly. It will not actually change this value. Only
> emulate this change towards drivers.
> 
> I will try to come up with a more detailed list later this week.

I reviewed the MC logic in mlxsw again and while I found some things
that can be improved, I don't think major simplification can happen
there.

Regarding "central model of what is behaviorally expected". IMO, the
best thing is to make sure that all the implementations pass tests that
codify what is to be expected. Given that the model is the Linux bridge,
the tests should of course pass with veth pairs.

We have some tests under tools/testing/selftests/net/forwarding/, but
not so much for MC. However, even if such tests were to be contributed,
would you be able to run them on your hardware? I remember that in the
past you complained about the number of required ports. Is there
something you can do about it? How many ports are available on your
platforms?

Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute

[Ido Schimmel]

On Sun, Jun 23, 2019 at 10:44:27AM +0300, Ido Schimmel wrote:
> On Sun, Jun 23, 2019 at 08:26:05AM +0100, Russell King - ARM Linux admin wrote:
> > On Sun, Jun 23, 2019 at 07:09:52AM +0000, Ido Schimmel wrote:
> > > When multicast snooping is enabled unregistered multicast traffic should
> > > only be flooded to mrouter ports.
> > 
> > Given that IPv6 relies upon multicast working, and multicast snooping
> > is a kernel configuration option, and MLD messages will only be sent
> > when whenever the configuration on the target changes, and there may
> > not be a multicast querier in the system, who does that ensure that
> > IPv6 can work on a bridge where the kernel configured and built with
> > multicast snooping enabled?
> 
> See commit b00589af3b04 ("bridge: disable snooping if there is no
> querier"). I think that's unfortunate behavior that we need because
> multicast snooping is enabled by default. If it weren't enabled by
> default, then anyone enabling it would also make sure there's a querier
> in the network.

Linus, Nik,

I brought this problem in the past, but we didn't reach a solution, so
I'll try again :)

The problem:

Even if multicast snooping is enabled, the bridge driver will flood
multicast packets to all the ports if no querier was detected on the
link. The querier states (IPv4 & IPv6) are not currently reflected to
switchdev drivers which means that the hardware data path will only
flood unregistered multicast packets to mrouter ports (which can be an
empty list).

In default configurations (where multicast snooping is enabled and the
bridge querier is disabled), this can prevent IPv6 ping from passing, as
there are no mrouter ports and there is no MDB entry corresponding to
the solicited-node multicast address.

Is there anything we can do about it? Enable the bridge querier if no
other querier was detected? Commit c5c23260594c ("bridge: Add
multicast_querier toggle and disable queries by default") disabled
queries by default, but I'm only suggesting to turn them on if no other
querier was detected on the link. Do you think it's still a problem?

I would like to avoid having drivers take the querier state into account
as it will only complicate things further.

Thanks

Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute

[Andrew Lunn]

> We have some tests under tools/testing/selftests/net/forwarding/, but
> not so much for MC. However, even if such tests were to be contributed,
> would you be able to run them on your hardware? I remember that in the
> past you complained about the number of required ports.

Hi Ido

Most devices have 4 or 5 ports. Some have more, some less. Given that
the current tests need a second port for every port under test, this
limits you to 2 ports under test. For IGMP snooping and MC in general,
i don't think that is enough. You need one port as the source of the
MC traffic, one port with a host interested in the traffic, and one
port without interest in the traffic. You can then test that traffic
is correctly forwarded and blocked.

The testing we do with DSA makes use of a test host with 4 interfaces,
and connect them one to one to the switch. That gives us 4 ports under
test, and so that gives us many more possibilities for running
interesting tests.

      Andrew

Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute

[Linus Lüssing]

On Sat, Jun 29, 2019 at 07:29:45PM +0300, Ido Schimmel wrote:
> I would like to avoid having drivers take the querier state into account
> as it will only complicate things further.

I absolutely share your pain. Initially in the early prototypes of
multicast awareness in batman-adv we did not consider the querier state.
And doing so later did indeed complicate the code a good bit in batman-adv
(together with the IGMP/MLD suppression issues). I would have loved to
avoid that.


> Is there anything we can do about it? Enable the bridge querier if no
> other querier was detected? Commit c5c23260594c ("bridge: Add
> multicast_querier toggle and disable queries by default") disabled
> queries by default, but I'm only suggesting to turn them on if no other
> querier was detected on the link. Do you think it's still a problem?

As soon as you start becoming the querier, you will not be able to reliably
detect anymore whether you are the only querier candidate.

If any random Linux host using a bridge device were potentially becoming
a querier, that would cause quite some trouble when this host is
behind some bad, bottleneck connection. This host will receive
all multicast traffic, not just IGMP/MLD reports. And with a
congested connection and then unreliable IGMP/MLD, multicast would
become unreliable overall in this domain. So it's important that
your querier is not running in the "dark, remote, dusty closet" of
your network (topologically speaking).

> On Sun, Jun 23, 2019 at 10:44:27AM +0300, Ido Schimmel wrote:
> > See commit b00589af3b04 ("bridge: disable snooping if there is no
> > querier"). I think that's unfortunate behavior that we need because
> > multicast snooping is enabled by default. If it weren't enabled by
> > default, then anyone enabling it would also make sure there's a querier
> > in the network.

I do not quite understand that point. In a way, that's what we
have right now, isn't it? By default it's disabled, because by
default there is no querier on the link. So anyone wanting to use
multicast snooping will need to make sure there's a querier in the
network.


Overall I think the querier (election) mechanism in the standards could
need an update. While the lowest-address first might have
worked well back then, in uniform, fully wired networks where the
position of the querier did not matter, this is not a good
solution anymore in networks involving wireless, dynamic connections.
Especially in wireless mesh networks this is a bit of an issue for
us. Ideally, the querier mechanism were dismissed in favour of simply
unsolicited, periodic IGMP/MLD reports...

But of course, updating IETF standards is no solution for now. 

While more complicated, it would not be impossible to consider the
querier state, would it? I mean you probably already need to
consider the case of a user disabling multicast snooping during
runtime, right? So similarly, you could react to appearing or
disappearing queriers?

Cheers, Linus

Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute

[Nikolay Aleksandrov]

On 30/06/2019 19:56, Linus Lüssing wrote:
> On Sat, Jun 29, 2019 at 07:29:45PM +0300, Ido Schimmel wrote:
>> I would like to avoid having drivers take the querier state into account
>> as it will only complicate things further.
> 
> I absolutely share your pain. Initially in the early prototypes of
> multicast awareness in batman-adv we did not consider the querier state.
> And doing so later did indeed complicate the code a good bit in batman-adv
> (together with the IGMP/MLD suppression issues). I would have loved to
> avoid that.
> 
> 
>> Is there anything we can do about it? Enable the bridge querier if no
>> other querier was detected? Commit c5c23260594c ("bridge: Add
>> multicast_querier toggle and disable queries by default") disabled
>> queries by default, but I'm only suggesting to turn them on if no other
>> querier was detected on the link. Do you think it's still a problem?
> 
> As soon as you start becoming the querier, you will not be able to reliably
> detect anymore whether you are the only querier candidate.
> 
> If any random Linux host using a bridge device were potentially becoming
> a querier, that would cause quite some trouble when this host is
> behind some bad, bottleneck connection. This host will receive
> all multicast traffic, not just IGMP/MLD reports. And with a
> congested connection and then unreliable IGMP/MLD, multicast would
> become unreliable overall in this domain. So it's important that
> your querier is not running in the "dark, remote, dusty closet" of
> your network (topologically speaking).
> 

+1
We definitely don't want random hosts becoming queriers

>> On Sun, Jun 23, 2019 at 10:44:27AM +0300, Ido Schimmel wrote:
>>> See commit b00589af3b04 ("bridge: disable snooping if there is no
>>> querier"). I think that's unfortunate behavior that we need because
>>> multicast snooping is enabled by default. If it weren't enabled by
>>> default, then anyone enabling it would also make sure there's a querier
>>> in the network.
> 
> I do not quite understand that point. In a way, that's what we
> have right now, isn't it? By default it's disabled, because by
> default there is no querier on the link. So anyone wanting to use
> multicast snooping will need to make sure there's a querier in the
> network.
> 

Indeed, also you could create the bridge with explicit mcast parameters if you need
different behaviour on start. Unfortunately I think you'll have to handle
the querier state.

> 
> Overall I think the querier (election) mechanism in the standards could
> need an update. While the lowest-address first might have
> worked well back then, in uniform, fully wired networks where the
> position of the querier did not matter, this is not a good
> solution anymore in networks involving wireless, dynamic connections.
> Especially in wireless mesh networks this is a bit of an issue for
> us. Ideally, the querier mechanism were dismissed in favour of simply
> unsolicited, periodic IGMP/MLD reports...
> 
> But of course, updating IETF standards is no solution for now. 
> 
> While more complicated, it would not be impossible to consider the
> querier state, would it? I mean you probably already need to
> consider the case of a user disabling multicast snooping during
> runtime, right? So similarly, you could react to appearing or
> disappearing queriers?
> 
> Cheers, Linus
> 

Thanks,
 Nik

Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute

[Ido Schimmel]

On Sun, Jun 30, 2019 at 06:56:01PM +0200, Linus Lüssing wrote:
> > On Sun, Jun 23, 2019 at 10:44:27AM +0300, Ido Schimmel wrote:
> > > See commit b00589af3b04 ("bridge: disable snooping if there is no
> > > querier"). I think that's unfortunate behavior that we need because
> > > multicast snooping is enabled by default. If it weren't enabled by
> > > default, then anyone enabling it would also make sure there's a querier
> > > in the network.
> 
> I do not quite understand that point. In a way, that's what we
> have right now, isn't it? By default it's disabled, because by
> default there is no querier on the link. So anyone wanting to use
> multicast snooping will need to make sure there's a querier in the
> network.

Hi Linus,

Querier state is not reflected to drivers ATM, so drivers believe the
bridge is multicast aware and unregistered multicast packets are only
flooded to mrouter ports. Hosts that are silent (because there is no
querier) never get the traffic addressed to them (f.e., IPv6 neighbour
solicitation).

> Overall I think the querier (election) mechanism in the standards could
> need an update. While the lowest-address first might have
> worked well back then, in uniform, fully wired networks where the
> position of the querier did not matter, this is not a good
> solution anymore in networks involving wireless, dynamic connections.
> Especially in wireless mesh networks this is a bit of an issue for
> us. Ideally, the querier mechanism were dismissed in favour of simply
> unsolicited, periodic IGMP/MLD reports...
> 
> But of course, updating IETF standards is no solution for now. 
> 
> While more complicated, it would not be impossible to consider the
> querier state, would it? I mean you probably already need to
> consider the case of a user disabling multicast snooping during
> runtime, right? 

Sure, this is implemented.

> So similarly, you could react to appearing or disappearing queriers?

Yes, but it's a bit more complicated since we need to differentiate
between IPv4 and IPv6. If the bridge is multicast aware, but there is
only IPv4 querier on the link, then:

1. All the IPv6 MDB entries need to be removed from the device. At least
in mlxsw, we do not have a way to ignore only IPv6 entries. From the
device's perspective, an MDB entry is just a multicast DMAC with a
bitmap of ports packets should be replicated to.

2. We need to split the flood tables used for IPv4 and IPv6 unregistered
multicast packets. For IPv4, packets should only be flooded to mrouter
ports whereas for IPv6 packets should be flooded to all the member
ports.

Do you differentiate between IPv4 and IPv6 in batman-adv?

> Cheers, Linus

Thanks for the feedback!

Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute

[Vivien Didelot]

Hi Ido,

On Sun, 23 Jun 2019 07:09:52 +0000, Ido Schimmel <idosch@mellanox.com> wrote:
> > Russell, Ido, Florian, so far I understand that a multicast-unaware
> > bridge must flood unknown traffic everywhere (CPU included);
> > and a multicast-aware bridge must only flood its ports if their
> > mcast_flood is on, and known traffic targeting the bridge must be
> > offloaded accordingly. Do you guys agree with this?
> 
> When multicast snooping is enabled unregistered multicast traffic should
> only be flooded to mrouter ports.

I've figured out that this is what I need to prevent the flooding of undesired
multicast traffic to the CPU port of the switch. The bridge itself has a
multicast_router attribute which can be disabled, that is when I should drop
unknown multicast traffic.

However with SWITCHDEV_ATTR_ID_BRIDGE_MROUTER implemented, this
attribute is always called with .mrouter=0, regardless the value of
/sys/class/net/br0/bridge/multicast_router. Do I miss something here?


Thanks,

	Vivien

Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute

[Ido Schimmel]

On Wed, Jul 03, 2019 at 01:13:08AM +0200, Linus Lüssing wrote:
> Hi Ido,
> 
> > Do you differentiate between IPv4 and IPv6 in batman-adv?
> 
> For most things, yes: The querier state is kept separately for
> IPv4 and IPv6. And we do have something like a "router node"
> flag to signalize that a node needs all multicast traffic, which
> is split into IPv4 and IPv6.
> 
> The "MDB" equivalent in batman-adv (multicast entries in our "TT",
> translation table) are on MAC address base right now, not on an IP
> address base yet, so that sounds similar to what you do in mlxsw?

Yes.

> Regarding querier state, we periodically ask the
> bridge via "br_multicast_has_querier_anywhere(dev, ETH_P_IP)"
> and "br_multicast_has_querier_anywhere(dev, ETH_P_IPV6)".
> 
> (Something more event based with handler functions would probably
> be nicer, but this was the easier thing to start with.)

Thanks for the reference. Will check the code. I believe that we will
add switchdev notifications for querier state change, so it might be
useful for you as well.

> > 1. All the IPv6 MDB entries need to be removed from the device. At least
> > in mlxsw, we do not have a way to ignore only IPv6 entries. From the
> > device's perspective, an MDB entry is just a multicast DMAC with a
> > bitmap of ports packets should be replicated to.
> 
> Ah, I see, yes. We had a similar "issue". Initially we just always
> added any multicast entry into our translation table offered by
> the IP stack or bridge, no matter what a querier state or "router
> node" state said. Which would have led to a node receiving two
> copies of a multicast packet if it were both a querier or router
> and were also having a listener announced via IGMP/MLD.
> 
> So there we also just recently changed that, to filter out
> IPv6 multicast TT entries if this node were also announcing itself as
> an MLD querier or IPv6 router. And same, independently for
> IPv4/IGMP.

This is actually not a problem with mlxsw. The ports a packet should be
replicated to are represented using a bitmap. It does not matter if we
set the bit because it has an MDB entry or because it is an mrouter
port. And obviously it does not matter if we set it twice :)

> > 2. We need to split the flood tables used for IPv4 and IPv6 unregistered
> > multicast packets. For IPv4, packets should only be flooded to mrouter
> > ports whereas for IPv6 packets should be flooded to all the member
> > ports.
> 
> This one I do not fully understand yet. Why don't you apply the
> "flood to all ports" (in the no IGMP querier present case)
> for IPv4, too?
> 
> Sure, for IPv4 nothing "essential" will break, as IPv4 unicast
> does not rely on multicast (contrary to IPv6, due to NDP, as you
> mentioned). But still there would be potential multicast packet loss
> for a 239.x.x.x listener on the local link, for instance, wouldn't
> there?
> 
> 
> If I'm not mistaken, we do not apply differing behaviour for IPv4
> vs. IPv6 in the bridge either and would flood on all ports for IPv4
> with no querier present, too.

I think I was not clear, so I will explain again. I was referring to a
situation where IPv4 has a querier, but IPv6 does not. In this case, the
bridge will flood IPv4 unregistered multicast packets to mrouter ports
only. On the other hand, IPv6 unregistered multicast packets will be
flooded to all the ports. Based on my reading of the code, this is
controlled by 'mcast_hit' in br_handle_frame_finish().

In mlxsw, each packet type (e.g., unknown unicast, IPvX unregistered
multicast) is associated with a flood table (basically a huge matrix,
where row corresponds to VLAN and column corresponds to a port). If we
are to handle the case where IPv4 unregistered multicast packets need to
be flooded to mrouter ports only, whereas IPv6 unregistered multicast
packets need to be flooded to all the ports, then we need to use a
separate flood table for each.

Alternatively, we can use the same flood table, but flood to all the
ports if IPv4 or IPv6 querier is missing. I do not think anything will
break, it is just very efficient. This seems to be allowed by RFC 4541
(Section 2.1.2):

"If a switch receives an unregistered packet, it must forward that
packet on all ports to which an IGMP router is attached. A switch may
default to forwarding unregistered packets on all ports."

> Regards, Linus

Linus, thanks a lot for the great feedback. I really appreciate it!

Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute

[Ido Schimmel]

On Fri, Jul 05, 2019 at 12:01:49PM -0400, Vivien Didelot wrote:
> Hi Ido,
> 
> On Sun, 23 Jun 2019 07:09:52 +0000, Ido Schimmel <idosch@mellanox.com> wrote:
> > > Russell, Ido, Florian, so far I understand that a multicast-unaware
> > > bridge must flood unknown traffic everywhere (CPU included);
> > > and a multicast-aware bridge must only flood its ports if their
> > > mcast_flood is on, and known traffic targeting the bridge must be
> > > offloaded accordingly. Do you guys agree with this?
> > 
> > When multicast snooping is enabled unregistered multicast traffic should
> > only be flooded to mrouter ports.
> 
> I've figured out that this is what I need to prevent the flooding of undesired
> multicast traffic to the CPU port of the switch. The bridge itself has a
> multicast_router attribute which can be disabled, that is when I should drop
> unknown multicast traffic.
> 
> However with SWITCHDEV_ATTR_ID_BRIDGE_MROUTER implemented, this
> attribute is always called with .mrouter=0, regardless the value of
> /sys/class/net/br0/bridge/multicast_router. Do I miss something here?

Hi Vivien,

I just checked this and it seems to work as expected:

# echo 2 > /sys/class/net/br0/bridge/multicast_router

We get a notification with mrouter=1 to mlxsw

# echo 0 > /sys/class/net/br0/bridge/multicast_router

We get a notification with mrouter=0 to mlxsw