[RFC,1/3] net: switchdev: do not propagate bridge updates across bridges

When configuring a tree of independent bridges, propagating changes
from the upper bridge across a bridge master to the lower bridge
ports brings surprises.

For example, a lower bridge may have vlan filtering enabled.  It
may have a vlan interface attached to the bridge master, which may
then be incorporated into another bridge.  As soon as the lower
bridge vlan interface is attached to the upper bridge, the lower
bridge has vlan filtering disabled.

This occurs because switchdev recursively applies its changes to
all lower devices no matter what.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 net/switchdev/switchdev.c | 9 +++++++++
 1 file changed, 9 insertions(+)

On Sun, Dec 22, 2019 at 07:24:03PM +0000, Russell King wrote:
> When configuring a tree of independent bridges, propagating changes
> from the upper bridge across a bridge master to the lower bridge
> ports brings surprises.
> 
> For example, a lower bridge may have vlan filtering enabled.  It
> may have a vlan interface attached to the bridge master, which may
> then be incorporated into another bridge.  As soon as the lower
> bridge vlan interface is attached to the upper bridge, the lower
> bridge has vlan filtering disabled.

Interesting topology :) The change looks OK to me. I'll add the patch to
our internal tree and let it go through regression to make sure I didn't
miss anything. Will report the results tomorrow.

> 
> This occurs because switchdev recursively applies its changes to
> all lower devices no matter what.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>  net/switchdev/switchdev.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
> index 3a1d428c1336..d881e5e4a889 100644
> --- a/net/switchdev/switchdev.c
> +++ b/net/switchdev/switchdev.c
> @@ -475,6 +475,9 @@ static int __switchdev_handle_port_obj_add(struct net_device *dev,
>  	 * necessary to go through this helper.
>  	 */
>  	netdev_for_each_lower_dev(dev, lower_dev, iter) {
> +		if (netif_is_bridge_master(lower_dev))
> +			continue;
> +
>  		err = __switchdev_handle_port_obj_add(lower_dev, port_obj_info,
>  						      check_cb, add_cb);
>  		if (err && err != -EOPNOTSUPP)
> @@ -526,6 +529,9 @@ static int __switchdev_handle_port_obj_del(struct net_device *dev,
>  	 * necessary to go through this helper.
>  	 */
>  	netdev_for_each_lower_dev(dev, lower_dev, iter) {
> +		if (netif_is_bridge_master(lower_dev))
> +			continue;
> +
>  		err = __switchdev_handle_port_obj_del(lower_dev, port_obj_info,
>  						      check_cb, del_cb);
>  		if (err && err != -EOPNOTSUPP)
> @@ -576,6 +582,9 @@ static int __switchdev_handle_port_attr_set(struct net_device *dev,
>  	 * necessary to go through this helper.
>  	 */
>  	netdev_for_each_lower_dev(dev, lower_dev, iter) {
> +		if (netif_is_bridge_master(lower_dev))
> +			continue;
> +
>  		err = __switchdev_handle_port_attr_set(lower_dev, port_attr_info,
>  						       check_cb, set_cb);
>  		if (err && err != -EOPNOTSUPP)
> -- 
> 2.20.1
>

On Tue, Dec 24, 2019 at 10:39:33AM +0200, Ido Schimmel wrote:
> On Sun, Dec 22, 2019 at 07:24:03PM +0000, Russell King wrote:
> > When configuring a tree of independent bridges, propagating changes
> > from the upper bridge across a bridge master to the lower bridge
> > ports brings surprises.
> > 
> > For example, a lower bridge may have vlan filtering enabled.  It
> > may have a vlan interface attached to the bridge master, which may
> > then be incorporated into another bridge.  As soon as the lower
> > bridge vlan interface is attached to the upper bridge, the lower
> > bridge has vlan filtering disabled.
> 
> Interesting topology :) The change looks OK to me. I'll add the patch to
> our internal tree and let it go through regression to make sure I didn't
> miss anything. Will report the results tomorrow.

Reviewed-by: Ido Schimmel <idosch@mellanox.com>
Tested-by: Ido Schimmel <idosch@mellanox.com>

Thanks!