Commit 2b0a5688 authored by Vladimir Oltean's avatar Vladimir Oltean Committed by David S. Miller
Browse files

net: switchdev: fix FDB entries towards foreign ports not getting propagated to us 



The newly introduced switchdev_handle_fdb_{add,del}_to_device helpers
solved a problem but introduced another one. They have a severe design
bug: they do not propagate FDB events on foreign interfaces to us, i.e.
this use case:

         br0
        /   \
       /     \
      /       \
     /         \
   swp0       eno0
(switchdev)  (foreign)

when an address is learned on eno0, what is supposed to happen is that
this event should also be propagated towards swp0. Somehow I managed to
convince myself that this did work correctly, but obviously it does not.

The trouble with foreign interfaces is that we must reach a switchdev
net_device pointer through a foreign net_device that has no direct
upper/lower relationship with it. So we need to do exploratory searching
through the lower interfaces of the foreign net_device's bridge upper
(to reach swp0 from eno0, we must check its upper, br0, for lower
interfaces that pass the check_cb and foreign_dev_check_cb). This is
something that the previous code did not do, it just assumed that "dev"
will become a switchdev interface at some point, somehow, probably by
magic.

With this patch, assisted address learning on the CPU port works again
in DSA:

ip link add br0 type bridge
ip link set swp0 master br0
ip link set eno0 master br0
ip link set br0 up

[   46.708929] mscc_felix 0000:00:00.5 swp0: Adding FDB entry towards eno0, addr 00:04:9f:05:f4:ab vid 0 as host address

Fixes: 8ca07176 ("net: switchdev: introduce a fanout helper for SWITCHDEV_FDB_{ADD,DEL}_TO_DEVICE")
Reported-by: default avatarEric Woudstra <ericwouds@gmail.com>
Signed-off-by: default avatarVladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
parent f796fcd6

net/switchdev/switchdev.c

+142 −72
Original line number Diff line number Diff line
@@ -378,6 +378,56 @@ int call_switchdev_blocking_notifiers(unsigned long val, struct net_device *dev, 
}

EXPORT_SYMBOL_GPL(call_switchdev_blocking_notifiers);



struct switchdev_nested_priv {

	bool (*check_cb)(const struct net_device *dev);

	bool (*foreign_dev_check_cb)(const struct net_device *dev,

				     const struct net_device *foreign_dev);

	const struct net_device *dev;

	struct net_device *lower_dev;

};



static int switchdev_lower_dev_walk(struct net_device *lower_dev,

				    struct netdev_nested_priv *priv)

{

	struct switchdev_nested_priv *switchdev_priv = priv->data;

	bool (*foreign_dev_check_cb)(const struct net_device *dev,

				     const struct net_device *foreign_dev);

	bool (*check_cb)(const struct net_device *dev);

	const struct net_device *dev;



	check_cb = switchdev_priv->check_cb;

	foreign_dev_check_cb = switchdev_priv->foreign_dev_check_cb;

	dev = switchdev_priv->dev;



	if (check_cb(lower_dev) && !foreign_dev_check_cb(lower_dev, dev)) {

		switchdev_priv->lower_dev = lower_dev;

		return 1;

	}



	return 0;

}



static struct net_device *

switchdev_lower_dev_find(struct net_device *dev,

			 bool (*check_cb)(const struct net_device *dev),

			 bool (*foreign_dev_check_cb)(const struct net_device *dev,

						      const struct net_device *foreign_dev))

{

	struct switchdev_nested_priv switchdev_priv = {

		.check_cb = check_cb,

		.foreign_dev_check_cb = foreign_dev_check_cb,

		.dev = dev,

		.lower_dev = NULL,

	};

	struct netdev_nested_priv priv = {

		.data = &switchdev_priv,

	};



	netdev_walk_all_lower_dev_rcu(dev, switchdev_lower_dev_walk, &priv);



	return switchdev_priv.lower_dev;

}



static int __switchdev_handle_fdb_add_to_device(struct net_device *dev,

		const struct net_device *orig_dev,

		const struct switchdev_notifier_fdb_info *fdb_info,
@@ -392,37 +442,18 @@ static int __switchdev_handle_fdb_add_to_device(struct net_device *dev, 
				  const struct switchdev_notifier_fdb_info *fdb_info))

{

	const struct switchdev_notifier_info *info = &fdb_info->info;

	struct net_device *lower_dev;

	struct net_device *br, *lower_dev;

	struct list_head *iter;

	int err = -EOPNOTSUPP;



	if (check_cb(dev)) {

		/* Handle FDB entries on foreign interfaces as FDB entries

		 * towards the software bridge.

		 */

		if (foreign_dev_check_cb && foreign_dev_check_cb(dev, orig_dev)) {

			struct net_device *br = netdev_master_upper_dev_get_rcu(dev);



			if (!br || !netif_is_bridge_master(br))

				return 0;



			/* No point in handling FDB entries on a foreign bridge */

			if (foreign_dev_check_cb(dev, br))

				return 0;



			return __switchdev_handle_fdb_add_to_device(br, orig_dev,

								    fdb_info, check_cb,

								    foreign_dev_check_cb,

								    add_cb, lag_add_cb);

		}



	if (check_cb(dev))

		return add_cb(dev, orig_dev, info->ctx, fdb_info);

	}



	/* If we passed over the foreign check, it means that the LAG interface

	 * is offloaded.

	 */

	if (netif_is_lag_master(dev)) {

		if (!switchdev_lower_dev_find(dev, check_cb, foreign_dev_check_cb))

			goto maybe_bridged_with_us;



		/* This is a LAG interface that we offload */

		if (!lag_add_cb)

			return -EOPNOTSUPP;
@@ -432,11 +463,24 @@ static int __switchdev_handle_fdb_add_to_device(struct net_device *dev, 
	/* Recurse through lower interfaces in case the FDB entry is pointing

	 * towards a bridge device.

	 */

	if (netif_is_bridge_master(dev)) {

		if (!switchdev_lower_dev_find(dev, check_cb, foreign_dev_check_cb))

			return 0;



		/* This is a bridge interface that we offload */

		netdev_for_each_lower_dev(dev, lower_dev, iter) {

			/* Do not propagate FDB entries across bridges */

			if (netif_is_bridge_master(lower_dev))

				continue;



			/* Bridge ports might be either us, or LAG interfaces

			 * that we offload.

			 */

			if (!check_cb(lower_dev) &&

			    !switchdev_lower_dev_find(lower_dev, check_cb,

						      foreign_dev_check_cb))

				continue;



			err = __switchdev_handle_fdb_add_to_device(lower_dev, orig_dev,

								   fdb_info, check_cb,

								   foreign_dev_check_cb,
@@ -445,7 +489,23 @@ static int __switchdev_handle_fdb_add_to_device(struct net_device *dev, 
				return err;

		}



	return err;

		return 0;

	}



maybe_bridged_with_us:

	/* Event is neither on a bridge nor a LAG. Check whether it is on an

	 * interface that is in a bridge with us.

	 */

	br = netdev_master_upper_dev_get_rcu(dev);

	if (!br || !netif_is_bridge_master(br))

		return 0;



	if (!switchdev_lower_dev_find(br, check_cb, foreign_dev_check_cb))

		return 0;



	return __switchdev_handle_fdb_add_to_device(br, orig_dev, fdb_info,

						    check_cb, foreign_dev_check_cb,

						    add_cb, lag_add_cb);

}



int switchdev_handle_fdb_add_to_device(struct net_device *dev,
@@ -487,37 +547,18 @@ static int __switchdev_handle_fdb_del_to_device(struct net_device *dev, 
				  const struct switchdev_notifier_fdb_info *fdb_info))

{

	const struct switchdev_notifier_info *info = &fdb_info->info;

	struct net_device *lower_dev;

	struct net_device *br, *lower_dev;

	struct list_head *iter;

	int err = -EOPNOTSUPP;



	if (check_cb(dev)) {

		/* Handle FDB entries on foreign interfaces as FDB entries

		 * towards the software bridge.

		 */

		if (foreign_dev_check_cb && foreign_dev_check_cb(dev, orig_dev)) {

			struct net_device *br = netdev_master_upper_dev_get_rcu(dev);



			if (!br || !netif_is_bridge_master(br))

				return 0;



			/* No point in handling FDB entries on a foreign bridge */

			if (foreign_dev_check_cb(dev, br))

				return 0;



			return __switchdev_handle_fdb_del_to_device(br, orig_dev,

								    fdb_info, check_cb,

								    foreign_dev_check_cb,

								    del_cb, lag_del_cb);

		}



	if (check_cb(dev))

		return del_cb(dev, orig_dev, info->ctx, fdb_info);

	}



	/* If we passed over the foreign check, it means that the LAG interface

	 * is offloaded.

	 */

	if (netif_is_lag_master(dev)) {

		if (!switchdev_lower_dev_find(dev, check_cb, foreign_dev_check_cb))

			goto maybe_bridged_with_us;



		/* This is a LAG interface that we offload */

		if (!lag_del_cb)

			return -EOPNOTSUPP;
@@ -527,11 +568,24 @@ static int __switchdev_handle_fdb_del_to_device(struct net_device *dev, 
	/* Recurse through lower interfaces in case the FDB entry is pointing

	 * towards a bridge device.

	 */

	if (netif_is_bridge_master(dev)) {

		if (!switchdev_lower_dev_find(dev, check_cb, foreign_dev_check_cb))

			return 0;



		/* This is a bridge interface that we offload */

		netdev_for_each_lower_dev(dev, lower_dev, iter) {

			/* Do not propagate FDB entries across bridges */

			if (netif_is_bridge_master(lower_dev))

				continue;



			/* Bridge ports might be either us, or LAG interfaces

			 * that we offload.

			 */

			if (!check_cb(lower_dev) &&

			    !switchdev_lower_dev_find(lower_dev, check_cb,

						      foreign_dev_check_cb))

				continue;



			err = __switchdev_handle_fdb_del_to_device(lower_dev, orig_dev,

								   fdb_info, check_cb,

								   foreign_dev_check_cb,
@@ -540,7 +594,23 @@ static int __switchdev_handle_fdb_del_to_device(struct net_device *dev, 
				return err;

		}



	return err;

		return 0;

	}



maybe_bridged_with_us:

	/* Event is neither on a bridge nor a LAG. Check whether it is on an

	 * interface that is in a bridge with us.

	 */

	br = netdev_master_upper_dev_get_rcu(dev);

	if (!br || !netif_is_bridge_master(br))

		return 0;



	if (!switchdev_lower_dev_find(br, check_cb, foreign_dev_check_cb))

		return 0;



	return __switchdev_handle_fdb_del_to_device(br, orig_dev, fdb_info,

						    check_cb, foreign_dev_check_cb,

						    del_cb, lag_del_cb);

}



int switchdev_handle_fdb_del_to_device(struct net_device *dev,