Timing of host-joined bridge multicast groups with switchdev

Timing of host-joined bridge multicast groups with switchdev

[Vladimir Oltean]

Hi,

I have udhcpcd in my system and this is configured to bring interfaces
up as soon as they are created.

I create a bridge as follows:

ip link add br0 type bridge

As soon as I create the bridge and udhcpcd brings it up, I have some
other crap (avahi) that starts sending some random IPv6 packets to
advertise some local services, and from there, the br0 bridge joins the
following IPv6 groups:

33:33:ff:6d:c1:9c vid 0
33:33:00:00:00:6a vid 0
33:33:00:00:00:fb vid 0

br_dev_xmit
-> br_multicast_rcv
   -> br_ip6_multicast_add_group
      -> __br_multicast_add_group
         -> br_multicast_host_join
            -> br_mdb_notify

This is all fine, but inside br_mdb_notify we have br_mdb_switchdev_host
hooked up, and switchdev will attempt to offload the host joined groups
to an empty list of ports. Of course nobody offloads them.

Then when we add a port to br0:

ip link set swp0 master br0

the bridge doesn't replay the host-joined MDB entries from br_add_if ->
new_nbp -> br_multicast_add_port (should it?), and eventually the host
joined addresses expire, and a switchdev notification for deleting it is
emitted, but surprise, the original addition was already completely missed.

What to do?

Thanks,
-Vladimir

Re: Timing of host-joined bridge multicast groups with switchdev

[Ido Schimmel]

On Tue, Feb 23, 2021 at 07:37:53PM +0200, Vladimir Oltean wrote:
> Hi,
> 
> I have udhcpcd in my system and this is configured to bring interfaces
> up as soon as they are created.
> 
> I create a bridge as follows:
> 
> ip link add br0 type bridge
> 
> As soon as I create the bridge and udhcpcd brings it up, I have some
> other crap (avahi) that starts sending some random IPv6 packets to
> advertise some local services, and from there, the br0 bridge joins the
> following IPv6 groups:
> 
> 33:33:ff:6d:c1:9c vid 0
> 33:33:00:00:00:6a vid 0
> 33:33:00:00:00:fb vid 0
> 
> br_dev_xmit
> -> br_multicast_rcv
>    -> br_ip6_multicast_add_group
>       -> __br_multicast_add_group
>          -> br_multicast_host_join
>             -> br_mdb_notify
> 
> This is all fine, but inside br_mdb_notify we have br_mdb_switchdev_host
> hooked up, and switchdev will attempt to offload the host joined groups
> to an empty list of ports. Of course nobody offloads them.
> 
> Then when we add a port to br0:
> 
> ip link set swp0 master br0
> 
> the bridge doesn't replay the host-joined MDB entries from br_add_if ->
> new_nbp -> br_multicast_add_port (should it?), and eventually the host
> joined addresses expire, and a switchdev notification for deleting it is
> emitted, but surprise, the original addition was already completely missed.
> 
> What to do?

For route offload you get a dump of all the existing routes when you
register your notifier. It's a bit different with bridge because you
don't care about existing bridges when you just initialize your driver.

We had a similar issue with VXLAN because its FDB can be populated and
only then attached to a bridge that you offload. Check
vxlan_fdb_replay(). Probably need to introduce something similar for
FDB/MDB entries.

Re: Timing of host-joined bridge multicast groups with switchdev

[Vladimir Oltean]

On Tue, Feb 23, 2021 at 07:56:22PM +0200, Ido Schimmel wrote:
> For route offload you get a dump of all the existing routes when you
> register your notifier. It's a bit different with bridge because you
> don't care about existing bridges when you just initialize your driver.
>
> We had a similar issue with VXLAN because its FDB can be populated and
> only then attached to a bridge that you offload. Check
> vxlan_fdb_replay(). Probably need to introduce something similar for
> FDB/MDB entries.

So you would be in favor of a driver-voluntary 'pull' type of approach
at bridge join, instead of the bridge 'pushing' the addresses?

That's all fine, except when we'll have more than 3 switchdev drivers,
how do we expect to manage all this complexity duplicated in many places
in the kernel, instead of having it in a central place? Are there corner
cases I'm missing which make the 'push' approach impractical?

Re: Timing of host-joined bridge multicast groups with switchdev

[Ido Schimmel]

On Tue, Feb 23, 2021 at 08:02:36PM +0200, Vladimir Oltean wrote:
> On Tue, Feb 23, 2021 at 07:56:22PM +0200, Ido Schimmel wrote:
> > For route offload you get a dump of all the existing routes when you
> > register your notifier. It's a bit different with bridge because you
> > don't care about existing bridges when you just initialize your driver.
> >
> > We had a similar issue with VXLAN because its FDB can be populated and
> > only then attached to a bridge that you offload. Check
> > vxlan_fdb_replay(). Probably need to introduce something similar for
> > FDB/MDB entries.
> 
> So you would be in favor of a driver-voluntary 'pull' type of approach
> at bridge join, instead of the bridge 'pushing' the addresses?
> 
> That's all fine, except when we'll have more than 3 switchdev drivers,
> how do we expect to manage all this complexity duplicated in many places
> in the kernel, instead of having it in a central place? Are there corner
> cases I'm missing which make the 'push' approach impractical?

Not sure. It needs to be scheduled when the driver is ready to handle
it. In br_add_if() after netdev_master_upper_dev_link() is probably a
good place.

It also needs to be done once and not every time another port joins the
bridge. This can be done using the port's parent ID, similar to what we
are already doing with the offload forward mark in
nbp_switchdev_mark_set().

But I'm not sure how we replay it only for a single notifier block. I'm
not familiar with setups where you have more than one listener let alone
more than one that is interested in notifications from a specific
bridge, so maybe it is OK to just replay it for all the listeners. But I
would prefer to avoid it if we can.

Re: Timing of host-joined bridge multicast groups with switchdev

[Vladimir Oltean]

On Tue, Feb 23, 2021 at 09:29:09PM +0200, Ido Schimmel wrote:
> On Tue, Feb 23, 2021 at 08:02:36PM +0200, Vladimir Oltean wrote:
> > On Tue, Feb 23, 2021 at 07:56:22PM +0200, Ido Schimmel wrote:
> > > For route offload you get a dump of all the existing routes when you
> > > register your notifier. It's a bit different with bridge because you
> > > don't care about existing bridges when you just initialize your driver.
> > >
> > > We had a similar issue with VXLAN because its FDB can be populated and
> > > only then attached to a bridge that you offload. Check
> > > vxlan_fdb_replay(). Probably need to introduce something similar for
> > > FDB/MDB entries.
> >
> > So you would be in favor of a driver-voluntary 'pull' type of approach
> > at bridge join, instead of the bridge 'pushing' the addresses?
> >
> > That's all fine, except when we'll have more than 3 switchdev drivers,
> > how do we expect to manage all this complexity duplicated in many places
> > in the kernel, instead of having it in a central place? Are there corner
> > cases I'm missing which make the 'push' approach impractical?
>
> Not sure. It needs to be scheduled when the driver is ready to handle
> it. In br_add_if() after netdev_master_upper_dev_link() is probably a
> good place.
>
> It also needs to be done once and not every time another port joins the
> bridge. This can be done using the port's parent ID, similar to what we
> are already doing with the offload forward mark in
> nbp_switchdev_mark_set().
>
> But I'm not sure how we replay it only for a single notifier block. I'm
> not familiar with setups where you have more than one listener let alone
> more than one that is interested in notifications from a specific
> bridge, so maybe it is OK to just replay it for all the listeners. But I
> would prefer to avoid it if we can.

At least with a driver-initiated pull, this seems to work:

-----------------------------[ cut here ]-----------------------------
From 13cb5ccbe35f64cfabe7dea3f76c8bc778cff9dc Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Tue, 23 Feb 2021 21:45:08 +0200
Subject: [PATCH] net: bridge: add a function that replays port and host-joined
 mdb entries

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 include/linux/if_bridge.h |   3 +
 include/net/switchdev.h   |   1 +
 net/bridge/br_mdb.c       | 115 ++++++++++++++++++++++++++++++++++++++
 net/dsa/slave.c           |   1 +
 4 files changed, 120 insertions(+)

diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
index b979005ea39c..d1190e2984bc 100644
--- a/include/linux/if_bridge.h
+++ b/include/linux/if_bridge.h
@@ -95,6 +95,9 @@ static inline bool br_multicast_router(const struct net_device *dev)
 }
 #endif
 
+int br_mdb_replay(struct net_device *br_dev, struct net_device *dev,
+		  struct notifier_block *nb);
+
 #if IS_ENABLED(CONFIG_BRIDGE) && IS_ENABLED(CONFIG_BRIDGE_VLAN_FILTERING)
 bool br_vlan_enabled(const struct net_device *dev);
 int br_vlan_get_pvid(const struct net_device *dev, u16 *p_pvid);
diff --git a/include/net/switchdev.h b/include/net/switchdev.h
index b7fc7d0f54e2..8c3218177136 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h
@@ -68,6 +68,7 @@ enum switchdev_obj_id {
 };
 
 struct switchdev_obj {
+	struct list_head list;
 	struct net_device *orig_dev;
 	enum switchdev_obj_id id;
 	u32 flags;
diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index 8846c5bcd075..72978c881e11 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -235,6 +235,121 @@ static int __mdb_fill_info(struct sk_buff *skb,
 	return -EMSGSIZE;
 }
 
+static int br_mdb_replay_one(struct notifier_block *nb, struct net_device *dev,
+			     struct switchdev_obj_port_mdb *mdb)
+{
+	struct switchdev_notifier_port_obj_info obj_info = {
+		.info = {
+			.dev = dev,
+		},
+		.obj = &mdb->obj,
+	};
+	int err;
+
+	err = nb->notifier_call(nb, SWITCHDEV_PORT_OBJ_ADD, &obj_info);
+	return notifier_to_errno(err);
+}
+
+static int br_mdb_queue_one(struct list_head *mdb_list,
+			    enum switchdev_obj_id id,
+			    struct net_bridge_mdb_entry *mp,
+			    struct net_device *orig_dev)
+{
+	struct switchdev_obj_port_mdb *mdb;
+
+	mdb = kzalloc(sizeof(*mdb), GFP_ATOMIC);
+	if (!mdb)
+		return -ENOMEM;
+
+	mdb->obj.id = id;
+	mdb->obj.orig_dev = orig_dev;
+	mdb->vid = mp->addr.vid;
+
+	if (mp->addr.proto == htons(ETH_P_IP))
+		ip_eth_mc_map(mp->addr.dst.ip4, mdb->addr);
+#if IS_ENABLED(CONFIG_IPV6)
+	else
+		ipv6_eth_mc_map(&mp->addr.dst.ip6, mdb->addr);
+#endif
+
+	list_add_tail(&mdb->obj.list, mdb_list);
+
+	return 0;
+}
+
+int br_mdb_replay(struct net_device *br_dev, struct net_device *dev,
+		  struct notifier_block *nb)
+{
+	struct net_bridge_mdb_entry *mp;
+	struct switchdev_obj *obj, *tmp;
+	struct list_head mdb_list;
+	struct net_bridge *br;
+	int err = 0;
+
+	ASSERT_RTNL();
+
+	INIT_LIST_HEAD(&mdb_list);
+
+	if (!netif_is_bridge_master(br_dev))
+		return -EINVAL;
+
+	if (!netif_is_bridge_port(dev))
+		return -EINVAL;
+
+	br = netdev_priv(br_dev);
+
+	if (!br_opt_get(br, BROPT_MULTICAST_ENABLED))
+		return 0;
+
+	rcu_read_lock();
+
+	hlist_for_each_entry_rcu(mp, &br->mdb_list, mdb_node) {
+		struct net_bridge_port_group __rcu **pp;
+		struct net_bridge_port_group *p;
+
+		if (mp->host_joined) {
+			err = br_mdb_queue_one(&mdb_list,
+					       SWITCHDEV_OBJ_ID_HOST_MDB,
+					       mp, br_dev);
+			if (err) {
+				rcu_read_unlock();
+				goto out_free_mdb;
+			}
+		}
+
+		for (pp = &mp->ports; (p = rcu_dereference(*pp)) != NULL;
+		     pp = &p->next) {
+			if (p->key.port->dev != dev)
+				continue;
+
+			err = br_mdb_queue_one(&mdb_list,
+					       SWITCHDEV_OBJ_ID_PORT_MDB,
+					       mp, dev);
+			if (err) {
+				rcu_read_unlock();
+				goto out_free_mdb;
+			}
+		}
+	}
+
+	rcu_read_unlock();
+
+	list_for_each_entry(obj, &mdb_list, list) {
+		err = br_mdb_replay_one(nb, dev, SWITCHDEV_OBJ_PORT_MDB(obj));
+		if (err)
+			goto out_free_mdb;
+	}
+
+out_free_mdb:
+	list_for_each_entry_safe(obj, tmp, &mdb_list, list) {
+		list_del(&obj->list);
+		kfree(SWITCHDEV_OBJ_PORT_MDB(obj));
+	}
+
+	return err;
+}
+EXPORT_SYMBOL(br_mdb_replay);
+
 static int br_mdb_fill_info(struct sk_buff *skb, struct netlink_callback *cb,
 			    struct net_device *dev)
 {
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 43e403ac70d5..9052ff5efab7 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -2070,6 +2070,7 @@ static int dsa_slave_changeupper(struct net_device *dev,
 			err = dsa_port_bridge_join(dp, info->upper_dev);
 			if (!err)
 				dsa_bridge_mtu_normalization(dp);
+			err = br_mdb_replay(info->upper_dev, dev, &dsa_slave_switchdev_blocking_notifier);
 			err = notifier_from_errno(err);
 		} else {
 			dsa_port_bridge_leave(dp, info->upper_dev);
-- 
2.25.1

-----------------------------[ cut here ]-----------------------------

I am just not sure why I need to emit the notification only once per
ASIC. Currently, SWITCHDEV_OBJ_ID_HOST_MDB is emitted for all bridge
ports, so the callers need to reference-count it anyway. As for the port
mdb entries, I am filtering the entries towards just a single port when
that joins. So I think this is okay.

Re: Timing of host-joined bridge multicast groups with switchdev

[Vladimir Oltean]

On Tue, Feb 23, 2021 at 09:49:08PM +0200, Vladimir Oltean wrote:
> > But I'm not sure how we replay it only for a single notifier block. I'm
> > not familiar with setups where you have more than one listener let alone
> > more than one that is interested in notifications from a specific
> > bridge, so maybe it is OK to just replay it for all the listeners. But I
> > would prefer to avoid it if we can.
> 
> At least with a driver-initiated pull, this seems to work:
...
> I am just not sure why I need to emit the notification only once per
> ASIC. Currently, SWITCHDEV_OBJ_ID_HOST_MDB is emitted for all bridge
> ports, so the callers need to reference-count it anyway. As for the port
> mdb entries, I am filtering the entries towards just a single port when
> that joins. So I think this is okay.

Sorry, I'm a bit more slow today.

So there seems to be no easy way to get access to the notifier block
that the new port uses, instead we can only call the entire notifier
chain from the bridge layer. I guess I'll have to defer that problem
until enough drivers make use of the pull mechanism that it becomes
unbearable then.

Re: [PATCH] net: bridge: add a function that replays port and host-joined

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3866 bytes --]

Hi Vladimir,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.11 next-20210223]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Vladimir-Oltean/net-bridge-add-a-function-that-replays-port-and-host-joined/20210224-035325
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 3b9cdafb5358eb9f3790de2f728f765fef100731
config: powerpc-randconfig-r003-20210223 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project c9439ca36342fb6013187d0a69aef92736951476)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install powerpc cross compiling tool for clang build
        # apt-get install binutils-powerpc-linux-gnu
        # https://github.com/0day-ci/linux/commit/ec816a5f014e0f0faacfe0462dcb1eeff54d8e50
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Vladimir-Oltean/net-bridge-add-a-function-that-replays-port-and-host-joined/20210224-035325
        git checkout ec816a5f014e0f0faacfe0462dcb1eeff54d8e50
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> net/dsa/slave.c:1959:47: error: use of undeclared identifier 'dsa_slave_switchdev_blocking_notifier'
                           err = br_mdb_replay(info->upper_dev, dev, &dsa_slave_switchdev_blocking_notifier);
                                                                      ^
   1 error generated.


vim +/dsa_slave_switchdev_blocking_notifier +1959 net/dsa/slave.c

  1947	
  1948	static int dsa_slave_changeupper(struct net_device *dev,
  1949					 struct netdev_notifier_changeupper_info *info)
  1950	{
  1951		struct dsa_port *dp = dsa_slave_to_port(dev);
  1952		int err = NOTIFY_DONE;
  1953	
  1954		if (netif_is_bridge_master(info->upper_dev)) {
  1955			if (info->linking) {
  1956				err = dsa_port_bridge_join(dp, info->upper_dev);
  1957				if (!err)
  1958					dsa_bridge_mtu_normalization(dp);
> 1959				err = br_mdb_replay(info->upper_dev, dev, &dsa_slave_switchdev_blocking_notifier);
  1960				err = notifier_from_errno(err);
  1961			} else {
  1962				dsa_port_bridge_leave(dp, info->upper_dev);
  1963				err = NOTIFY_OK;
  1964			}
  1965		} else if (netif_is_lag_master(info->upper_dev)) {
  1966			if (info->linking) {
  1967				err = dsa_port_lag_join(dp, info->upper_dev,
  1968							info->upper_info);
  1969				if (err == -EOPNOTSUPP) {
  1970					NL_SET_ERR_MSG_MOD(info->info.extack,
  1971							   "Offloading not supported");
  1972					err = 0;
  1973				}
  1974				err = notifier_from_errno(err);
  1975			} else {
  1976				dsa_port_lag_leave(dp, info->upper_dev);
  1977				err = NOTIFY_OK;
  1978			}
  1979		} else if (is_hsr_master(info->upper_dev)) {
  1980			if (info->linking) {
  1981				err = dsa_port_hsr_join(dp, info->upper_dev);
  1982				if (err == -EOPNOTSUPP) {
  1983					NL_SET_ERR_MSG_MOD(info->info.extack,
  1984							   "Offloading not supported");
  1985					err = 0;
  1986				}
  1987				err = notifier_from_errno(err);
  1988			} else {
  1989				dsa_port_hsr_leave(dp, info->upper_dev);
  1990				err = NOTIFY_OK;
  1991			}
  1992		}
  1993	
  1994		return err;
  1995	}
  1996	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31418 bytes --]