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: <8f51bb33-c5a5-4046-93d6-f58e841256e5@gmail.com>
Date: Fri, 28 Mar 2025 11:53:21 -0400
From: Joseph Huang <joseph.huang.2024@...il.com>
To: Nikolay Aleksandrov <razor@...ckwall.org>,
 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/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);
+}

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;

?

Thanks,
Joseph