linux-kernel - Re: [Patch net-next 1/3] net: bridge: mcast: Add offload failed mdb flag

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <da181580-56bd-460c-b2bf-16ebb86bbff5@blackwall.org>
Date: Mon, 31 Mar 2025 15:51:54 +0300
From: Nikolay Aleksandrov <razor@...ckwall.org>
To: Joseph Huang <joseph.huang.2024@...il.com>,
 Joseph Huang <Joseph.Huang@...min.com>, netdev@...r.kernel.org
Cc: Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Roopa Prabhu <roopa@...dia.com>, Simon Horman <horms@...nel.org>,
 linux-kernel@...r.kernel.org, bridge@...ts.linux.dev
Subject: Re: [Patch net-next 1/3] net: bridge: mcast: Add offload failed mdb
 flag

On 3/28/25 17:53, Joseph Huang wrote:
> On 3/27/2025 6:52 PM, Nikolay Aleksandrov wrote:
>> On 3/27/25 00:38, Joseph Huang wrote:
>>> On 3/21/2025 4:19 AM, Nikolay Aleksandrov wrote:
>>>>> @@ -516,11 +513,14 @@ static void br_switchdev_mdb_complete(struct 
>>>>> net_device *dev, int err, void *pri
>>>>>            pp = &p->next) {
>>>>>           if (p->key.port != port)
>>>>>               continue;
>>>>> -        p->flags |= MDB_PG_FLAGS_OFFLOAD;
>>>>> +
>>>>> +        if (err)
>>>>> +            p->flags |= MDB_PG_FLAGS_OFFLOAD_FAILED;
>>>>> +        else
>>>>> +            p->flags |= MDB_PG_FLAGS_OFFLOAD;
>>>>
>>>> These two should be mutually exclusive, either it's offloaded or it 
>>>> failed an offload,
>>>> shouldn't be possible to have both set. I'd recommend adding some 
>>>> helper that takes
>>>> care of that.
>>>
>>> It is true that these two are mutually exclusive, but strictly 
>>> speaking there are four types of entries:
>>>
>>> 1. Entries which are not offload-able (i.e., the ports are not backed 
>>> by switchdev)
>>> 2. Entries which are being offloaded, but results yet unknown
>>> 3. Entries which are successfully offloaded, and
>>> 4. Entries which failed to be offloaded
>>>
>>> Even if we ignore the ones which are being offloaded (type 2 is 
>>> transient), we still need two flags, otherwise we won't be able to 
>>> tell type 1 from type 4 entries.
>>>
>>> If we need two flags anyway, having separate flags for type 3 and 
>>> type 4 simplifies the logic.
>>>
>>> Or did I misunderstood your comments?
>>>
>>> Thanks,
>>> Joseph
>>
>> I think you misunderstood me, I don't mind having the two flags. :)
> 
> Got it. Thanks.
> 
>> My point is that they must be managed correctly and shouldn't be allowed
>> to be set simultaneously.
>>
>> Cheers,
>>   Nik
>>
> 
> Helper function like this?
> 
> +static void set_mdb_pg_offload_flags(bool err, u8 *flags)
> +{
> +    *flags &= ~(MDB_PG_FLAGS_OFFLOAD | MDB_PG_FLAGS_OFFLOAD_FAILED);
> +    *flags |= (err ? MDB_PG_FLAGS_OFFLOAD_FAILED : MDB_PG_FLAGS_OFFLOAD);
> +}

This could work, but I have to see how it aligns with the rest of the
code to be able to answer well. Also why not just pass the pg?
Please also choose another helper name, e.g.
br_multicast_set_pg_offload_flags() or something in these lines. You
can check br_private.h for other helpers to get an idea.

> 
> and then from the call site
> 
> -        p->flags |= MDB_PG_FLAGS_OFFLOAD;
> +        set_mdb_pg_offload_flags(err, &p->flags);
> 
> ?
> 
> Or simply clearing the flags in-line:
> 
> -        p->flags |= MDB_PG_FLAGS_OFFLOAD;
> +        p->flags &= ~(MDB_PG_FLAGS_OFFLOAD | MDB_PG_FLAGS_OFFLOAD_FAILED);
> +
> +        if (err)
> +            p->flags |= MDB_PG_FLAGS_OFFLOAD_FAILED;
> +        else
> +            p->flags |= MDB_PG_FLAGS_OFFLOAD;
> 
> ?

I'd prefer using a  helper. Thanks.

> 
> Thanks,
> Joseph

Cheers,
  Nik