| 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
| ||
|
Message-ID: <accf2693-caee-4576-bc4d-6f1533ec18e5@blackwall.org>
Date: Tue, 1 Apr 2025 16:37:24 +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 2/3] net: bridge: mcast: Notify on offload flag
change
On 4/1/25 15:49, Nikolay Aleksandrov wrote:
> On 3/31/25 23:11, Joseph Huang wrote:
>> On 3/21/2025 4:47 AM, Nikolay Aleksandrov wrote:
>>>> diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
>>>> index 68dccc2ff7b1..5b09cfcdf3f3 100644
>>>> --- a/net/bridge/br_switchdev.c
>>>> +++ b/net/bridge/br_switchdev.c
>>>> @@ -504,20 +504,41 @@ static void br_switchdev_mdb_complete(struct net_device *dev, int err, void *pri
>>>> struct net_bridge_mdb_entry *mp;
>>>> struct net_bridge_port *port = data->port;
>>>> struct net_bridge *br = port->br;
>>>> + bool offload_changed = false;
>>>> + bool failed_changed = false;
>>>> + u8 notify;
>>>> spin_lock_bh(&br->multicast_lock);
>>>> mp = br_mdb_ip_get(br, &data->ip);
>>>> if (!mp)
>>>> goto out;
>>>> +
>>>> + notify = br->multicast_ctx.multicast_mdb_notify_on_flag_change;
>>>
>>> let's not waste cycles if there was an error and notify == 0, please keep the original
>>> code path and avoid walking over the group ports.
>>
>> But we do want to keep the error flag so that the error shows up in 'bridge mdb show', right? Notify should only affect the real-time notifications, and not the error status itself.
>>
>
> Fair enough, sounds good.
>
>>>
>>>> +
>>>> for (pp = &mp->ports; (p = mlock_dereference(*pp, br)) != NULL;
>>>> pp = &p->next) {
>>>> if (p->key.port != port)
>>>> continue;
>>>> - if (err)
>>>> + if (err) {
>>>> + if (!(p->flags & MDB_PG_FLAGS_OFFLOAD_FAILED))
>>>> + failed_changed = true;
>>>> p->flags |= MDB_PG_FLAGS_OFFLOAD_FAILED;
>>>> - else
>>>> + } else {
>>>> + if (!(p->flags & MDB_PG_FLAGS_OFFLOAD))
>>>> + offload_changed = true;
>>>> p->flags |= MDB_PG_FLAGS_OFFLOAD;
>>>> + }
>>>> +
>>>> + if (notify == MDB_NOTIFY_ON_FLAG_CHANGE_DISABLE ||
>>>> + (!offload_changed && !failed_changed))
>>>> + continue;
>>>> +
>>>> + if (notify == MDB_NOTIFY_ON_FLAG_CHANGE_FAIL_ONLY &&
>>>> + !failed_changed)
>>>> + continue;
>>>> +
>>>> + br_mdb_flag_change_notify(br->dev, mp, p);
>>>
>>> This looks like a mess.. First you need to manage these flags properly as I wrote in my
>>> other reply, they must be mutually exclusive and you can do this in a helper. Also
>>> please read the old flags in the beginning, then check what flags changed, make a mask
>>> what flags are for notifications (again can come from a helper, it can be generated when
>>> the option changes so you don't compute it every time) and decide what to do if any of
>>> those flags changed.
>>> Note you have to keep proper flags state regardless of the notify option.
>>>
>>>> }
>>>> out:
>>>> spin_unlock_bh(&br->multicast_lock);
>>>
>>
>> How does this look:
>>
>> --- a/net/bridge/br_switchdev.c
>> +++ b/net/bridge/br_switchdev.c
>> @@ -496,6 +496,21 @@ struct br_switchdev_mdb_complete_info {
>> struct br_ip ip;
>> };
>>
>
> #define MDB_NOTIFY_FLAGS MDB_PG_FLAGS_OFFLOAD_FAILED
>
pardon me, you can drop this define as the flag is guarded by a specific
option so we don't always notify when we see it, you can check for it
explicitly below in changed_flags below...
>> +static void br_multicast_set_pg_offload_flags(int err,
>> + struct net_bridge_port_group *p)
>
> swap these two arguments please, since we don't use err you can probably
> rename it to "failed" and make it a bool
>
> alternatively if you prefer maybe rename it to
> br_multicast_set_pg_offload_flag() and pass the correct flag from the
> caller
> e.g. br_multicast_set_pg_offload_flag(pg, err ?
> MDB_PG_FLAGS_OFFLOAD_FAILED : MDB_PG_FLAGS_OFFLOAD)
>
> I don't mind either way.
>
>> +{
>> + p->flags &= ~(MDB_PG_FLAGS_OFFLOAD | MDB_PG_FLAGS_OFFLOAD_FAILED);
>> + p->flags |= (err ? MDB_PG_FLAGS_OFFLOAD_FAILED : MDB_PG_FLAGS_OFFLOAD);
>> +}
>> +
>> +static bool br_multicast_should_notify(struct net_bridge *br,
>
> hmm perhaps br_mdb_should_notify() to be more specific? I don't mind the
> current name, just a thought.
>
> also const br
>
>> + u8 old_flags, u8 new_flags)
>
> u8 changed_flags should suffice
>
>> +{
>> + return (br_boolopt_get(br, BR_BOOLOPT_FAILED_OFFLOAD_NOTIFICATION) &&
>> + ((old_flags & MDB_PG_FLAGS_OFFLOAD_FAILED) !=
>> + (new_flags & MDB_PG_FLAGS_OFFLOAD_FAILED)));
>
> if (changed_flags & MDB_NOTIFY_FLAGS)
... here just do an explicit check for the offload flag in changed_flags
instead of using a define, it is guarded by a specific option so it's ok
>
> also no need for the extra () around the whole statement
>
>> +}
>> +
>
> both of these helpers should go into br_private.h
>
>> static void br_switchdev_mdb_complete(struct net_device *dev, int err, void *priv)
>> {
>> struct br_switchdev_mdb_complete_info *data = priv;
>> @@ -504,23 +519,25 @@ static void br_switchdev_mdb_complete(struct net_device *dev, int err, void *pri
>> struct net_bridge_mdb_entry *mp;
>> struct net_bridge_port *port = data->port;
>> struct net_bridge *br = port->br;
>> -
>> - if (err)
>> - goto err;
>> + u8 old_flags;
>>
>> spin_lock_bh(&br->multicast_lock);
>> mp = br_mdb_ip_get(br, &data->ip);
>> if (!mp)
>> goto out;
>> for (pp = &mp->ports; (p = mlock_dereference(*pp, br)) != NULL;
>> pp = &p->next) {
>> if (p->key.port != port)
>> continue;
>> - p->flags |= MDB_PG_FLAGS_OFFLOAD;
>> +
>> + old_flags = p->flags;
>> + br_multicast_set_pg_offload_flags(err, p);
>> + if (br_multicast_should_notify(br, old_flags, p->flags))
>
> and here it would become:
> br_multicast_should_notify(br, old_flags ^ p->flags)
>
>> + br_mdb_flag_change_notify(br->dev, mp, p);
>> }
>> out:
>> spin_unlock_bh(&br->multicast_lock);
>> -err:
>> kfree(priv);
>> }
>>
>> Thanks,
>> Joseph
>
> Cheers,
> Nik
>
Powered by blists - more mailing lists