[PATCH net] net: bridge: allow users setting EXT_LEARN for user FDB entries

[PATCH net] net: bridge: allow users setting EXT_LEARN for user FDB entries

[Jonas Gorski]

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 <jonas.gorski@bisdn.de>
---
 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;
-- 
2.46.0


-- 
BISDN GmbH
Körnerstraße 7-10
10785 Berlin
Germany


Phone: 
+49-30-6108-1-6100


Managing Directors: 
Dr.-Ing. Hagen Woesner, Andreas 
Köpsel


Commercial register: 
Amtsgericht Berlin-Charlottenburg HRB 141569 
B
VAT ID No: DE283257294

Re: [PATCH net] net: bridge: allow users setting EXT_LEARN for user FDB entries

[Nikolay Aleksandrov]

On 30/08/2024 17:53, Jonas Gorski wrote:
> 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 <jonas.gorski@bisdn.de>
> ---
>  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

Re: [PATCH net] net: bridge: allow users setting EXT_LEARN for user FDB entries

[Ido Schimmel]

On Sat, Aug 31, 2024 at 11:31:50AM +0300, Nikolay Aleksandrov wrote:
> On 30/08/2024 17:53, Jonas Gorski wrote:
> > 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 <jonas.gorski@bisdn.de>
> > ---
> >  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;
                }

Re: [PATCH net] net: bridge: allow users setting EXT_LEARN for user FDB entries

[Nikolay Aleksandrov]

On 01/09/2024 14:54, Ido Schimmel wrote:
> On Sat, Aug 31, 2024 at 11:31:50AM +0300, Nikolay Aleksandrov wrote:
>> On 30/08/2024 17:53, Jonas Gorski wrote:
>>> 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 <jonas.gorski@bisdn.de>
>>> ---
>>>  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.

Re: [PATCH net] net: bridge: allow users setting EXT_LEARN for user FDB entries

[Jonas Gorski]

Am So., 1. Sept. 2024 um 14:25 Uhr schrieb Nikolay Aleksandrov
<razor@blackwall.org>:
>
> On 01/09/2024 14:54, Ido Schimmel wrote:
> > On Sat, Aug 31, 2024 at 11:31:50AM +0300, Nikolay Aleksandrov wrote:
> >> On 30/08/2024 17:53, Jonas Gorski wrote:
> >>> 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 <jonas.gorski@bisdn.de>
> >>> ---
> >>>  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.

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.

Best Regards,
Jonas

-- 
BISDN GmbH
Körnerstraße 7-10
10785 Berlin
Germany


Phone: 
+49-30-6108-1-6100


Managing Directors: 
Dr.-Ing. Hagen Woesner, Andreas 
Köpsel


Commercial register: 
Amtsgericht Berlin-Charlottenburg HRB 141569 
B
VAT ID No: DE283257294

Re: [PATCH net] net: bridge: allow users setting EXT_LEARN for user FDB entries

[Ido Schimmel]

On Mon, Sep 02, 2024 at 09:34:48AM +0200, Jonas Gorski wrote:
> Am So., 1. Sept. 2024 um 14:25 Uhr schrieb Nikolay Aleksandrov
> <razor@blackwall.org>:
> >
> > On 01/09/2024 14:54, Ido Schimmel wrote:
> > > On Sat, Aug 31, 2024 at 11:31:50AM +0300, Nikolay Aleksandrov wrote:
> > >> On 30/08/2024 17:53, Jonas Gorski wrote:
> > >>> 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 <jonas.gorski@bisdn.de>
> > >>> ---
> > >>>  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.

> 
> 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

Re: [PATCH net] net: bridge: allow users setting EXT_LEARN for user FDB entries

[Nikolay Aleksandrov]

On 9/2/24 17:59, Ido Schimmel wrote:
> On Mon, Sep 02, 2024 at 09:34:48AM +0200, Jonas Gorski wrote:
>> Am So., 1. Sept. 2024 um 14:25 Uhr schrieb Nikolay Aleksandrov
>> <razor@blackwall.org>:
>>>
>>> On 01/09/2024 14:54, Ido Schimmel wrote:
>>>> On Sat, Aug 31, 2024 at 11:31:50AM +0300, Nikolay Aleksandrov wrote:
>>>>> On 30/08/2024 17:53, Jonas Gorski wrote:
>>>>>> 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 <jonas.gorski@bisdn.de>
>>>>>> ---
>>>>>>  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.
> 
>>
>> 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