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@xxxxxxxxxxxxx>:
>>>
>>> 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@xxxxxxxx>
>>>>>> ---
>>>>>>  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