On 9/2/24 17:59, Ido Schimmel wrote:

On Mon, Sep 02, 2024 at 09:34:48AM +0200, Jonas Gorski wrote:

quoted

Am So., 1. Sept. 2024 um 14:25 Uhr schrieb Nikolay Aleksandrov
[off-list ref]:

quoted

On 01/09/2024 14:54, Ido Schimmel wrote:

quoted

On Sat, Aug 31, 2024 at 11:31:50AM +0300, Nikolay Aleksandrov wrote:

quoted

On 30/08/2024 17:53, Jonas Gorski wrote:

quoted

When userspace wants to take over a fdb entry by setting it as
EXTERN_LEARNED, we set both flags BR_FDB_ADDED_BY_EXT_LEARN and
BR_FDB_ADDED_BY_USER in br_fdb_external_learn_add().

If the bridge updates the entry later because its port changed, we clear
the BR_FDB_ADDED_BY_EXT_LEARN flag, but leave the BR_FDB_ADDED_BY_USER
flag set.

If userspace then wants to take over the entry again,
br_fdb_external_learn_add() sees that BR_FDB_ADDED_BY_USER and skips
setting the BR_FDB_ADDED_BY_EXT_LEARN flags, thus silently ignores the
update:

   if (test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags)) {
           /* Refresh entry */
           fdb->used = jiffies;
   } else if (!test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags)) {
           /* Take over SW learned entry */
           set_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags);
           modified = true;
   }

Fix this by relaxing the condition for setting BR_FDB_ADDED_BY_EXT_LEARN
by also allowing it if swdev_notify is true, which it will only be for
user initiated updates.

Fixes: 710ae7287737 ("net: bridge: Mark FDB entries that were added by user as such")
Signed-off-by: Jonas Gorski <redacted>
---
 net/bridge/br_fdb.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index c77591e63841..c5d9ae13a6fb 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c

@@ -1472,7 +1472,8 @@ int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
            if (test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags)) {
                    /* Refresh entry */
                    fdb->used = jiffies;
-           } else if (!test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags)) {
+           } else if (swdev_notify ||
+                      !test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags)) {
                    /* Take over SW learned entry */
                    set_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags);
                    modified = true;

This literally means if added_by_user || !added_by_user, so you can probably
rewrite that whole block to be more straight-forward with test_and_set_bit -
if it was already set then refresh, if it wasn't modified = true

Hi Nik,

You mean like this [1]?
I deleted the comment about "SW learned entry" since "extern_learn" flag
not being set does not necessarily mean the entry was learned by SW.

[1]

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index c77591e63841..ad7a42b505ef 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c

@@ -1469,12 +1469,10 @@ int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
                        modified = true;
                }

-               if (test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags)) {
+               if (test_and_set_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags)) {
                        /* Refresh entry */
                        fdb->used = jiffies;
-               } else if (!test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags)) {
-                       /* Take over SW learned entry */
-                       set_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags);
+               } else {
                        modified = true;
                }

Yeah, that's exactly what I meant. Since the added_by_user condition becomes
redundant we can just drop it.

br_fdb_external_learn_add() is called from two places; once when
userspace adds a EXT_LEARN flagged fdb entry (then swdev_nofity is
set), and once when a switchdev driver reports it has learned an entry
(then swdev_notify isn't).

AFAIU the previous condition was to prevent user fdb entries from
being taken over by hardware / switchdev events, which this would now
allow to happen. OTOH, the switchdev notifications are a statement of
fact, and the kernel really has a say into whether the hardware should
keep the entry learned, so not allowing entries to be marked as
learned by hardware would also result in a disconnect between hardware
and kernel.

The entries were already learned by the hardware and the kernel even
updated their destination in br_fdb_external_learn_add(), it is just
that it didn't set the EXT_LEARN flag on them, which seems like a
mistake.

quoted

My change was trying to accomodate for the former one, i.e. if the
user bit is set, only the user may mark it as EXT_LEARN, but not any
(switchdev) drivers.

I have no strong feelings about what I think is right, so if this is
the wanted direction, I can send a V2 doing that.

I prefer v2 as it means that an entry that was learned by the hardware
will now be marked as such regardless if it was previously added by user
space or not

+1
We were already in a bad situation, if anything this would make it
better. We can take care of added_by_user behaviour later.

Thanks,
 Nik