From 2b0a5688493ab0f54774a2f89d3d91ab238f7ab4 Mon Sep 17 00:00:00 2001 From: Vladimir Oltean Date: Thu, 22 Jul 2021 02:05:55 +0300 Subject: 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: 8ca07176ab00 ("net: switchdev: introduce a fanout helper for SWITCHDEV_FDB_{ADD,DEL}_TO_DEVICE") Reported-by: Eric Woudstra Signed-off-by: Vladimir Oltean Signed-off-by: David S. Miller --- net/switchdev/switchdev.c | 214 ++++++++++++++++++++++++++++++---------------- 1 file changed, 142 insertions(+), 72 deletions(-) (limited to 'net/switchdev') diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c index 42e88d3d66a7..0ae3478561f4 100644 --- a/net/switchdev/switchdev.c +++ b/net/switchdev/switchdev.c @@ -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,20 +463,49 @@ 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. */ - netdev_for_each_lower_dev(dev, lower_dev, iter) { - /* Do not propagate FDB entries across bridges */ - if (netif_is_bridge_master(lower_dev)) - continue; + 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, + add_cb, lag_add_cb); + if (err && err != -EOPNOTSUPP) + return err; + } - err = __switchdev_handle_fdb_add_to_device(lower_dev, orig_dev, - fdb_info, check_cb, - foreign_dev_check_cb, - add_cb, lag_add_cb); - if (err && err != -EOPNOTSUPP) - return err; + return 0; } - return err; +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,20 +568,49 @@ 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. */ - netdev_for_each_lower_dev(dev, lower_dev, iter) { - /* Do not propagate FDB entries across bridges */ - if (netif_is_bridge_master(lower_dev)) - continue; + 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, + del_cb, lag_del_cb); + if (err && err != -EOPNOTSUPP) + return err; + } - err = __switchdev_handle_fdb_del_to_device(lower_dev, orig_dev, - fdb_info, check_cb, - foreign_dev_check_cb, - del_cb, lag_del_cb); - if (err && err != -EOPNOTSUPP) - return err; + return 0; } - return err; +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, -- cgit v1.2.3-70-g09d2