Linux Ethernet Bridging - Mar 2023 - [Bridge] [PATCH v2 net-next 2/6] net: dsa: propagate flags down towards drivers

On Sat, Mar 18, 2023 at 03:10:06PM +0100, Hans J. Schultz
wrote:
> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
> index e5f156940c67..c07a2e225ae5 100644
> --- a/net/dsa/dsa.c
> +++ b/net/dsa/dsa.c
> @@ -626,6 +626,12 @@ static int dsa_switch_setup(struct dsa_switch *ds)
>  
>  	ds->configure_vlan_while_not_filtering = true;
>  
> +	/* Since dynamic FDB entries are legacy, all switch drivers should
> +	 * support the flag at least by just installing a static entry and
> +	 * letting the bridge age it.
> +	 */
> +	ds->supported_fdb_flags = DSA_FDB_FLAG_DYNAMIC;
I believe that switchdev has a structural problem in the fact that FDB
entries with flags that aren't interpreted by drivers (so they don't
know if those flags are set or unset) are still passed to the switchdev
notifier chains by default.

I don't believe that anybody used 'bridge fdb add <mac>
<dev> master dynamic"
while relying on a static FDB entry in the DSA offloaded data path.

Just like commit 6ab4c3117aec ("net: bridge: don't notify switchdev for
local FDB addresses"), we could deny that for stable kernels, and add
the correct interpretation of the flag in net-next.

Ido, Nikolay, Roopa, Jiri, thoughts?
> +
>  	err = ds->ops->setup(ds);
>  	if (err < 0)
>  		goto unregister_notifier;
By the way, there is a behavior change here.

Before:

$ ip link add br0 type bridge && ip link set br0 up
$ ip link set swp0 master br0 && ip link set swp0 up
$ bridge fdb add dev swp0 00:01:02:03:04:05 master dynamic
[   70.010181] mscc_felix 0000:00:00.5: felix_fdb_add: port 0 addr
00:01:02:03:04:05 vid 0
[   70.019105] mscc_felix 0000:00:00.5: felix_fdb_add: port 0 addr
00:01:02:03:04:05 vid 1
.... 5 minutes later
[  371.686935] mscc_felix 0000:00:00.5: felix_fdb_del: port 0 addr
00:01:02:03:04:05 vid 1
[  371.695449] mscc_felix 0000:00:00.5: felix_fdb_del: port 0 addr
00:01:02:03:04:05 vid 0
$ bridge fdb | grep 00:01:02:03:04:05

After:

$ ip link add br0 type bridge && ip link set br0 up
$ ip link set swp0 master br0 && ip link set swp0 up
$ bridge fdb add dev swp0 00:01:02:03:04:05 master dynamic
[  222.071492] mscc_felix 0000:00:00.5: felix_fdb_add: port 0 addr
00:01:02:03:04:05 vid 0 flags 0x1
[  222.081154] mscc_felix 0000:00:00.5: felix_fdb_add: port 0 addr
00:01:02:03:04:05 vid 1 flags 0x1
.... 5 minutes later
$ bridge fdb | grep 00:01:02:03:04:05
00:01:02:03:04:05 dev swp0 vlan 1 offload master br0 stale
00:01:02:03:04:05 dev swp0 offload master br0 stale
00:01:02:03:04:05 dev swp0 vlan 1 self
00:01:02:03:04:05 dev swp0 self

As you can see, the behavior is not identical, and it made more sense
before.

On Mon, Mar 27, 2023 at 14:52, Vladimir Oltean <olteanv at gmail.com>
wrote:
>
> By the way, there is a behavior change here.
>
> Before:
>
> $ ip link add br0 type bridge && ip link set br0 up
> $ ip link set swp0 master br0 && ip link set swp0 up
> $ bridge fdb add dev swp0 00:01:02:03:04:05 master dynamic
> [   70.010181] mscc_felix 0000:00:00.5: felix_fdb_add: port 0 addr
00:01:02:03:04:05 vid 0
> [   70.019105] mscc_felix 0000:00:00.5: felix_fdb_add: port 0 addr
00:01:02:03:04:05 vid 1
> .... 5 minutes later
> [  371.686935] mscc_felix 0000:00:00.5: felix_fdb_del: port 0 addr
00:01:02:03:04:05 vid 1
> [  371.695449] mscc_felix 0000:00:00.5: felix_fdb_del: port 0 addr
00:01:02:03:04:05 vid 0
> $ bridge fdb | grep 00:01:02:03:04:05
>
> After:
>
> $ ip link add br0 type bridge && ip link set br0 up
> $ ip link set swp0 master br0 && ip link set swp0 up
> $ bridge fdb add dev swp0 00:01:02:03:04:05 master dynamic
> [  222.071492] mscc_felix 0000:00:00.5: felix_fdb_add: port 0 addr
00:01:02:03:04:05 vid 0 flags 0x1
> [  222.081154] mscc_felix 0000:00:00.5: felix_fdb_add: port 0 addr
00:01:02:03:04:05 vid 1 flags 0x1
> .... 5 minutes later
> $ bridge fdb | grep 00:01:02:03:04:05
> 00:01:02:03:04:05 dev swp0 vlan 1 offload master br0 stale
> 00:01:02:03:04:05 dev swp0 offload master br0 stale
> 00:01:02:03:04:05 dev swp0 vlan 1 self
> 00:01:02:03:04:05 dev swp0 self
>
> As you can see, the behavior is not identical, and it made more sense
> before.
I see this is Felix Ocelot and there is no changes in this patchset that
affects Felix Ocelot. Thus I am quite sure the results will be the same
without this patchset, ergo it must be because of another patch. All
that is done here in the DSA layer is to pass on an extra field and add
an extra check that will always pass in the case of this flag.