[net] net: bridge: switchdev: don't notify FDB entries with "master dynamic"

  There is a structural problem in switchdev, where the flag bits in
struct switchdev_notifier_fdb_info (added_by_user, is_local etc) only
represent a simplified / denatured view of what's in struct
net_bridge_fdb_entry :: flags (BR_FDB_ADDED_BY_USER, BR_FDB_LOCAL etc).
Each time we want to pass more information about struct
net_bridge_fdb_entry :: flags to struct switchdev_notifier_fdb_info
(here, BR_FDB_STATIC), we find that FDB entries were already notified to
switchdev with no regard to this flag, and thus, switchdev drivers had
no indication whether the notified entries were static or not.

For example, this command:

ip link add br0 type bridge && ip link set swp0 master br0
bridge fdb add dev swp0 00:01:02:03:04:05 master dynamic

causes a struct net_bridge_fdb_entry to be passed to
br_switchdev_fdb_notify() which has a single flag set:
BR_FDB_ADDED_BY_USER.

This is further passed to the switchdev notifier chain, where interested
drivers have no choice but to assume this is a static FDB entry.
So currently, all drivers offload it to hardware as such.

bridge fdb get 00:01:02:03:04:05 dev swp0 master
00:01:02:03:04:05 dev swp0 offload master br0

The software FDB entry expires after the $ageing_time and the bridge
notifies its deletion as well, so it eventually disappears from hardware
too.

This is a problem, because it is actually desirable to start offloading
"master dynamic" FDB entries correctly, and this is how the current
incorrect behavior was discovered.

To see why the current behavior of "here's a static FDB entry when you
asked for a dynamic one" is incorrect, it is possible to imagine a
scenario like below, where this decision could lead to packet loss:

Step 1: management prepares FDB entries like this:

bridge fdb add dev swp0 ${MAC_A} master dynamic
bridge fdb add dev swp2 ${MAC_B} master dynamic

        br0
      /  |  \
     /   |   \
  swp0  swp1  swp2
   |           |
   A           B

Step 2: station A migrates to swp1 (assume that swp0's link doesn't flap
during that time so that the port isn't flushed, for example station A
was behind an intermediary switch):

        br0
      /  |  \
     /   |   \
  swp0  swp1  swp2
   |     |     |
         A     B

Whenever A wants to ping B, its packets will be autonomously forwarded
by the switch (because ${MAC_B} is known). So the software will never
see packets from ${MAC_A} as source address, and will never know it
needs to invalidate the dynamic FDB entry towards swp0. As for the
hardware FDB entry, that's static, it doesn't move when the station
roams.

So when B wants to reply to A's pings, the switch will forward those
replies to swp0 until the software bridge ages out its dynamic entry,
and that can cause connectivity loss for up to 5 minutes after roaming.

With a correctly offloaded dynamic FDB entry, the switch would update
its entry for ${MAC_A} to be towards swp1 as soon as it sees packets
from it (no need for CPU intervention).

Looking at tools/testing/selftests/net/forwarding/, there is no valid
use of the "bridge fdb add ... master dynamic" command there, so I am
fairly confident that no one used to rely on this behavior.

With the change in place, these FDB entries are no longer offloaded:

bridge fdb get 00:01:02:03:04:05 dev swp0 master
00:01:02:03:04:05 dev swp0 master br0

and this also constitutes a better way (assuming a backport to stable
kernels) for user space to determine whether the switchdev driver did
actually act upon the dynamic FDB entry or not.

Fixes: 6b26b51b1d13 ("net: bridge: Add support for notifying devices about FDB add/del")
Link: https://lore.kernel.org/netdev/20230327115206.jk5q5l753aoelwus@skbuf/
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/bridge/br_switchdev.c | 4 ++++
 1 file changed, 4 insertions(+)
  

On 4/10/2023 1:49 PM, Vladimir Oltean wrote:
> There is a structural problem in switchdev, where the flag bits in
> struct switchdev_notifier_fdb_info (added_by_user, is_local etc) only
> represent a simplified / denatured view of what's in struct
> net_bridge_fdb_entry :: flags (BR_FDB_ADDED_BY_USER, BR_FDB_LOCAL etc).
> Each time we want to pass more information about struct
> net_bridge_fdb_entry :: flags to struct switchdev_notifier_fdb_info
> (here, BR_FDB_STATIC), we find that FDB entries were already notified to
> switchdev with no regard to this flag, and thus, switchdev drivers had
> no indication whether the notified entries were static or not.
> 
> For example, this command:
> 
> ip link add br0 type bridge && ip link set swp0 master br0
> bridge fdb add dev swp0 00:01:02:03:04:05 master dynamic
> 
> causes a struct net_bridge_fdb_entry to be passed to
> br_switchdev_fdb_notify() which has a single flag set:
> BR_FDB_ADDED_BY_USER.
> 
> This is further passed to the switchdev notifier chain, where interested
> drivers have no choice but to assume this is a static FDB entry.
> So currently, all drivers offload it to hardware as such.
> 
> bridge fdb get 00:01:02:03:04:05 dev swp0 master
> 00:01:02:03:04:05 dev swp0 offload master br0
> 
> The software FDB entry expires after the $ageing_time and the bridge
> notifies its deletion as well, so it eventually disappears from hardware
> too.
> 
> This is a problem, because it is actually desirable to start offloading
> "master dynamic" FDB entries correctly, and this is how the current
> incorrect behavior was discovered.
> 
> To see why the current behavior of "here's a static FDB entry when you
> asked for a dynamic one" is incorrect, it is possible to imagine a
> scenario like below, where this decision could lead to packet loss:
> 
> Step 1: management prepares FDB entries like this:
> 
> bridge fdb add dev swp0 ${MAC_A} master dynamic
> bridge fdb add dev swp2 ${MAC_B} master dynamic
> 
>         br0
>       /  |  \
>      /   |   \
>   swp0  swp1  swp2
>    |           |
>    A           B
> 
> Step 2: station A migrates to swp1 (assume that swp0's link doesn't flap
> during that time so that the port isn't flushed, for example station A
> was behind an intermediary switch):
> 
>         br0
>       /  |  \
>      /   |   \
>   swp0  swp1  swp2
>    |     |     |
>          A     B
> 
> Whenever A wants to ping B, its packets will be autonomously forwarded
> by the switch (because ${MAC_B} is known). So the software will never
> see packets from ${MAC_A} as source address, and will never know it
> needs to invalidate the dynamic FDB entry towards swp0. As for the
> hardware FDB entry, that's static, it doesn't move when the station
> roams.
> 
> So when B wants to reply to A's pings, the switch will forward those
> replies to swp0 until the software bridge ages out its dynamic entry,
> and that can cause connectivity loss for up to 5 minutes after roaming.
> 
> With a correctly offloaded dynamic FDB entry, the switch would update
> its entry for ${MAC_A} to be towards swp1 as soon as it sees packets
> from it (no need for CPU intervention).
> 
> Looking at tools/testing/selftests/net/forwarding/, there is no valid
> use of the "bridge fdb add ... master dynamic" command there, so I am
> fairly confident that no one used to rely on this behavior.
> 
> With the change in place, these FDB entries are no longer offloaded:
> 
> bridge fdb get 00:01:02:03:04:05 dev swp0 master
> 00:01:02:03:04:05 dev swp0 master br0
> 
> and this also constitutes a better way (assuming a backport to stable
> kernels) for user space to determine whether the switchdev driver did
> actually act upon the dynamic FDB entry or not.
> 
> Fixes: 6b26b51b1d13 ("net: bridge: Add support for notifying devices about FDB add/del")
> Link: https://lore.kernel.org/netdev/20230327115206.jk5q5l753aoelwus@skbuf/
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Looks fine to me, but I'd like to see other switchdev experts reply.

Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
  

On Mon, Apr 10, 2023 at 11:49:51PM +0300, Vladimir Oltean wrote:
> There is a structural problem in switchdev, where the flag bits in
> struct switchdev_notifier_fdb_info (added_by_user, is_local etc) only
> represent a simplified / denatured view of what's in struct
> net_bridge_fdb_entry :: flags (BR_FDB_ADDED_BY_USER, BR_FDB_LOCAL etc).
> Each time we want to pass more information about struct
> net_bridge_fdb_entry :: flags to struct switchdev_notifier_fdb_info
> (here, BR_FDB_STATIC), we find that FDB entries were already notified to
> switchdev with no regard to this flag, and thus, switchdev drivers had
> no indication whether the notified entries were static or not.
> 
> For example, this command:
> 
> ip link add br0 type bridge && ip link set swp0 master br0
> bridge fdb add dev swp0 00:01:02:03:04:05 master dynamic
> 
> causes a struct net_bridge_fdb_entry to be passed to
> br_switchdev_fdb_notify() which has a single flag set:
> BR_FDB_ADDED_BY_USER.
> 
> This is further passed to the switchdev notifier chain, where interested
> drivers have no choice but to assume this is a static FDB entry.
> So currently, all drivers offload it to hardware as such.
> 
> bridge fdb get 00:01:02:03:04:05 dev swp0 master
> 00:01:02:03:04:05 dev swp0 offload master br0
> 
> The software FDB entry expires after the $ageing_time and the bridge
> notifies its deletion as well, so it eventually disappears from hardware
> too.
> 
> This is a problem, because it is actually desirable to start offloading
> "master dynamic" FDB entries correctly, and this is how the current
> incorrect behavior was discovered.
> 
> To see why the current behavior of "here's a static FDB entry when you
> asked for a dynamic one" is incorrect, it is possible to imagine a
> scenario like below, where this decision could lead to packet loss:
> 
> Step 1: management prepares FDB entries like this:
> 
> bridge fdb add dev swp0 ${MAC_A} master dynamic
> bridge fdb add dev swp2 ${MAC_B} master dynamic
> 
>         br0
>       /  |  \
>      /   |   \
>   swp0  swp1  swp2
>    |           |
>    A           B
> 
> Step 2: station A migrates to swp1 (assume that swp0's link doesn't flap
> during that time so that the port isn't flushed, for example station A
> was behind an intermediary switch):
> 
>         br0
>       /  |  \
>      /   |   \
>   swp0  swp1  swp2
>    |     |     |
>          A     B
> 
> Whenever A wants to ping B, its packets will be autonomously forwarded
> by the switch (because ${MAC_B} is known). So the software will never
> see packets from ${MAC_A} as source address, and will never know it
> needs to invalidate the dynamic FDB entry towards swp0. As for the
> hardware FDB entry, that's static, it doesn't move when the station
> roams.
> 
> So when B wants to reply to A's pings, the switch will forward those
> replies to swp0 until the software bridge ages out its dynamic entry,
> and that can cause connectivity loss for up to 5 minutes after roaming.
> 
> With a correctly offloaded dynamic FDB entry, the switch would update
> its entry for ${MAC_A} to be towards swp1 as soon as it sees packets
> from it (no need for CPU intervention).
> 
> Looking at tools/testing/selftests/net/forwarding/, there is no valid
> use of the "bridge fdb add ... master dynamic" command there, so I am
> fairly confident that no one used to rely on this behavior.

Yes, but there are tests that use "extern_learn". If you post a v2 that
takes "BR_FDB_ADDED_BY_EXT_LEARN" into account, then I can ask Petr to
run it through our regression and report back (not sure we will make it
to this week's PR though).

Thanks

> 
> With the change in place, these FDB entries are no longer offloaded:
> 
> bridge fdb get 00:01:02:03:04:05 dev swp0 master
> 00:01:02:03:04:05 dev swp0 master br0
> 
> and this also constitutes a better way (assuming a backport to stable
> kernels) for user space to determine whether the switchdev driver did
> actually act upon the dynamic FDB entry or not.
> 
> Fixes: 6b26b51b1d13 ("net: bridge: Add support for notifying devices about FDB add/del")
> Link: https://lore.kernel.org/netdev/20230327115206.jk5q5l753aoelwus@skbuf/
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  net/bridge/br_switchdev.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
> index de18e9c1d7a7..0ec3d5e5e77d 100644
> --- a/net/bridge/br_switchdev.c
> +++ b/net/bridge/br_switchdev.c
> @@ -148,6 +148,10 @@ br_switchdev_fdb_notify(struct net_bridge *br,
>  	if (test_bit(BR_FDB_LOCKED, &fdb->flags))
>  		return;
>  
> +	if (test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags) &&
> +	    !test_bit(BR_FDB_STATIC, &fdb->flags))
> +		return;
> +
>  	br_switchdev_fdb_populate(br, &item, fdb, NULL);
>  
>  	switch (type) {
> -- 
> 2.34.1
>
  

On Wed, Apr 12, 2023 at 05:15:03PM +0300, Ido Schimmel wrote:
> > Looking at tools/testing/selftests/net/forwarding/, there is no valid
> > use of the "bridge fdb add ... master dynamic" command there, so I am
> > fairly confident that no one used to rely on this behavior.
> 
> Yes, but there are tests that use "extern_learn". If you post a v2 that
> takes "BR_FDB_ADDED_BY_EXT_LEARN" into account, then I can ask Petr to
> run it through our regression and report back (not sure we will make it
> to this week's PR though).
> 
> Thanks

How are extern_learn FDB entries processed by spectrum's
SWITCHDEV_FDB_ADD_TO_DEVICE handler?
  

On Wed, Apr 12, 2023 at 05:27:33PM +0300, Vladimir Oltean wrote:
> How are extern_learn FDB entries processed by spectrum's
> SWITCHDEV_FDB_ADD_TO_DEVICE handler?

No different than "BR_FDB_STATIC", which is a bug I'm aware of and
intend to fix in net-next when I get the time (together with all the
other combinations enabled by the bridge). Entry has ageing disabled,
but can roam in which case it becomes age-able.

TBH, I think most devices don't handle "BR_FDB_STATIC" correctly. In the
Linux bridge, "BR_FDB_STATIC" only means ageing disabled. The entry can
still roam, but remains "static". I believe that in most devices out
there "static" means no roaming and no ageing which is equivalent to
"BR_FDB_STATIC | BR_FDB_STICKY". Mentioned in your commit message as
well: "As for the hardware FDB entry, that's static, it doesn't move
when the station roams."

As it stands, the situation is far from perfect, but the patch doesn't
solve a regression (always broken) and will introduce one. My suggestion
allows you to move forward and solve the "dynamic" case, so let's
proceed with that unless there's a better alternative.
  

On Wed, Apr 12, 2023 at 07:00:02PM +0300, Ido Schimmel wrote:
> On Wed, Apr 12, 2023 at 05:27:33PM +0300, Vladimir Oltean wrote:
> > How are extern_learn FDB entries processed by spectrum's
> > SWITCHDEV_FDB_ADD_TO_DEVICE handler?
> 
> No different than "BR_FDB_STATIC", which is a bug I'm aware of and
> intend to fix in net-next when I get the time (together with all the
> other combinations enabled by the bridge). Entry has ageing disabled,
> but can roam in which case it becomes age-able.
> 
> TBH, I think most devices don't handle "BR_FDB_STATIC" correctly. In the
> Linux bridge, "BR_FDB_STATIC" only means ageing disabled. The entry can
> still roam, but remains "static". I believe that in most devices out
> there "static" means no roaming and no ageing which is equivalent to
> "BR_FDB_STATIC | BR_FDB_STICKY". Mentioned in your commit message as
> well: "As for the hardware FDB entry, that's static, it doesn't move
> when the station roams."
> 
> As it stands, the situation is far from perfect, but the patch doesn't
> solve a regression (always broken) and will introduce one. My suggestion
> allows you to move forward and solve the "dynamic" case, so let's
> proceed with that unless there's a better alternative.

I'm not trying to accuse anybody, I just wanted to make sure I'm not
missing something (and surprise, I was).

The comment regarding BR_FDB_STATIC vs BR_FDB_STATIC | BR_FDB_STICKY
is interesting. This whole "hey, did you know you were never using the
bridge fdb flags correctly?" is starting to become a bit of a meme.

I'll send v2 with BR_FDB_ADDED_BY_EXT_LEARN not prevented from being
notified from switchdev.

Unless you have any objection, I won't send v2 like this:

	if (test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags) &&
	    !test_bit(BR_FDB_STATIC, &fdb->flags) &&
	    !test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags))
		return;

but like this:

	/* Entries with just the BR_FDB_ADDED_BY_USER flag set were created
	 * using 'bridge fdb add ... master dynamic'
	 */
	if (fdb->flags == BIT(BR_FDB_ADDED_BY_USER))
		return;

Thanks for the review and for pointing out the regression early.
  

On Wed, Apr 12, 2023 at 07:24:07PM +0300, Vladimir Oltean wrote:
> I'll send v2 with BR_FDB_ADDED_BY_EXT_LEARN not prevented from being
> notified from switchdev.
> 
> Unless you have any objection, I won't send v2 like this:
> 
> 	if (test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags) &&
> 	    !test_bit(BR_FDB_STATIC, &fdb->flags) &&
> 	    !test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags))
> 		return;
> 
> but like this:
> 
> 	/* Entries with just the BR_FDB_ADDED_BY_USER flag set were created
> 	 * using 'bridge fdb add ... master dynamic'
> 	 */
> 	if (fdb->flags == BIT(BR_FDB_ADDED_BY_USER))
> 		return;

LGTM. Please copy me on v2 and I will run it through regression. Will
try to report results before tomorrow's PR.

Thanks!
  

On Wed, Apr 12, 2023 at 07:49:17PM +0300, Ido Schimmel wrote:
> On Wed, Apr 12, 2023 at 07:24:07PM +0300, Vladimir Oltean wrote:
> > I'll send v2 with BR_FDB_ADDED_BY_EXT_LEARN not prevented from being
> > notified from switchdev.
> > 
> > Unless you have any objection, I won't send v2 like this:
> > 
> > 	if (test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags) &&
> > 	    !test_bit(BR_FDB_STATIC, &fdb->flags) &&
> > 	    !test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags))
> > 		return;
> > 
> > but like this:
> > 
> > 	/* Entries with just the BR_FDB_ADDED_BY_USER flag set were created
> > 	 * using 'bridge fdb add ... master dynamic'
> > 	 */
> > 	if (fdb->flags == BIT(BR_FDB_ADDED_BY_USER))
> > 		return;
> 
> LGTM. Please copy me on v2 and I will run it through regression. Will
> try to report results before tomorrow's PR.

Sorry, I won't send the v2 early; I need to think more about this and
I don't want to rush things. It's likely that the zero-day bug fix will
miss this week's PR.