| 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: <7eb3164c-7288-4b3b-9cee-75525607bead@blackwall.org>
Date: Tue, 1 Apr 2025 15:49:35 +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 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
> +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)
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