Re: [RFC PATCH v2 net-next 08/16] net: dsa: replay port and host-joined mdb entries when joining the bridge

On Fri, Mar 19, 2021 at 03:20:38PM -0700, Florian Fainelli wrote:

On 3/18/2021 4:18 PM, Vladimir Oltean wrote:

quoted

From: Vladimir Oltean <vladimir.oltean@nxp.com>

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)

How dare you ;)

Well, it comes preinstalled on my system, I don't need it, and it has
caused me nothing but trouble. So I think it has earned its title :D

quoted

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,
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.

The strategy to address this problem is to replay the MDB entries (both
the port ones and the host joined ones) when the new port joins the
bridge, similar to what vxlan_fdb_replay does (in that case, its FDB can
be populated and only then attached to a bridge that you offload).
However there are 2 possibilities: the addresses can be 'pushed' by the
bridge into the port, or the port can 'pull' them from the bridge.

Considering that in the general case, the new port can be really late to
the party, and there may have been many other switchdev ports that
already received the initial notification, we would like to avoid
delivering duplicate events to them, since they might misbehave. And
currently, the bridge calls the entire switchdev notifier chain, whereas
for replaying it should just call the notifier block of the new guy.
But the bridge doesn't know what is the new guy's notifier block, it
just knows where the switchdev notifier chain is. So for simplification,
we make this a driver-initiated pull for now, and the notifier block is
passed as an argument.

To emulate the calling context for mdb objects (deferred and put on the
blocking notifier chain), we must iterate under RCU protection through
the bridge's mdb entries, queue them, and only call them once we're out
of the RCU read-side critical section.

Suggested-by: Ido Schimmel <redacted>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 include/linux/if_bridge.h |  9 +++++
 net/bridge/br_mdb.c       | 84 +++++++++++++++++++++++++++++++++++++++
 net/dsa/dsa_priv.h        |  2 +
 net/dsa/port.c            |  6 +++
 net/dsa/slave.c           |  2 +-
 5 files changed, 102 insertions(+), 1 deletion(-)

diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
index ebd16495459c..4c25dafb013d 100644
--- a/include/linux/if_bridge.h
+++ b/include/linux/if_bridge.h

@@ -69,6 +69,8 @@ bool br_multicast_has_querier_anywhere(struct net_device *dev, int proto);
 bool br_multicast_has_querier_adjacent(struct net_device *dev, int proto);
 bool br_multicast_enabled(const struct net_device *dev);
 bool br_multicast_router(const struct net_device *dev);
+int br_mdb_replay(struct net_device *br_dev, struct net_device *dev,
+		  struct notifier_block *nb, struct netlink_ext_ack *extack);
 #else
 static inline int br_multicast_list_adjacent(struct net_device *dev,
 					     struct list_head *br_ip_list)

@@ -93,6 +95,13 @@ static inline bool br_multicast_router(const struct net_device *dev)
 {
 	return false;
 }
+static inline int br_mdb_replay(struct net_device *br_dev,
+				struct net_device *dev,
+				struct notifier_block *nb,
+				struct netlink_ext_ack *extack)
+{
+	return -EINVAL;

Should we return -EOPNOTUSPP such that this is not made fatal for DSA if
someone compiles its kernel with CONFIG_BRIDGE_IGMP_SNOOPING disabled?

Sure, I'll change the return values of the shims everywhere.

quoted

+}
 #endif

 #if IS_ENABLED(CONFIG_BRIDGE) && IS_ENABLED(CONFIG_BRIDGE_VLAN_FILTERING)

diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index 8846c5bcd075..23973186094c 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c

@@ -506,6 +506,90 @@ static void br_mdb_complete(struct net_device *dev, int err, void *priv)
 	kfree(priv);
 }

+static int br_mdb_replay_one(struct notifier_block *nb, struct net_device *dev,
+			     struct net_bridge_mdb_entry *mp, int obj_id,
+			     struct net_device *orig_dev,
+			     struct netlink_ext_ack *extack)
+{
+	struct switchdev_notifier_port_obj_info obj_info = {
+		.info = {
+			.dev = dev,
+			.extack = extack,
+		},
+	};
+	struct switchdev_obj_port_mdb mdb = {
+		.obj = {
+			.orig_dev = orig_dev,
+			.id = obj_id,
+		},
+		.vid = mp->addr.vid,
+	};
+	int err;
+
+	if (mp->addr.proto == htons(ETH_P_IP))
+		ip_eth_mc_map(mp->addr.dst.ip4, mdb.addr);
+#if IS_ENABLED(CONFIG_IPV6)
+	else if (mp->addr.proto == htons(ETH_P_IPV6))
+		ipv6_eth_mc_map(&mp->addr.dst.ip6, mdb.addr);
+#endif
+	else
+		ether_addr_copy(mdb.addr, mp->addr.dst.mac_addr);

How you would feel about re-using br_mdb_switchdev_host_port() here and
pass a 'type' value that is neither RTM_NEWDB nor RTM_DELDB just so you
don't have to duplicate that code here and we ensure it is in sync?

The trouble is that br_mdb_switchdev_host calls switchdev_port_obj_add,
and I think the agreement was that replayed events should be a silent,
one-to-one conversation via a direct call to the notifier block of the
interested driver, as opposed to a call to the entire notifier chain
which would make everybody else in the system see duplicates. This is
the reason why I duplicated mostly everything.