lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <aee25423-c9e4-4c3a-8990-d019f085467c@blackwall.org>
Date: Tue, 3 Sep 2024 10:27:41 +0300
From: Nikolay Aleksandrov <razor@...ckwall.org>
To: Ido Schimmel <idosch@...dia.com>, Jonas Gorski <jonas.gorski@...dn.de>
Cc: Roopa Prabhu <roopa@...dia.com>, "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Petr Machata <petrm@...lanox.com>,
 Ido Schimmel <idosch@...lanox.com>, bridge@...ts.linux.dev,
 netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net] net: bridge: allow users setting EXT_LEARN for user
 FDB entries

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@...ckwall.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@...dn.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


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ