linux-kernel - Re: [Patch net-next 2/3] net: bridge: mcast: Notify on offload flag change

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: <5d93f576-1d27-4d3f-8b37-0b2127260cca@gmail.com>
Date: Mon, 31 Mar 2025 16:11:50 -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 2/3] net: bridge: mcast: Notify on offload flag
 change

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.

> 
>> +
>>   	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;
  };

+static void br_multicast_set_pg_offload_flags(int err,
+                                             struct 
net_bridge_port_group *p)
+{
+       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,
+                                      u8 old_flags, u8 new_flags)
+{
+       return (br_boolopt_get(br, 
BR_BOOLOPT_FAILED_OFFLOAD_NOTIFICATION) &&
+               ((old_flags & MDB_PG_FLAGS_OFFLOAD_FAILED) !=
+               (new_flags & MDB_PG_FLAGS_OFFLOAD_FAILED)));
+}
+
  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))
+                       br_mdb_flag_change_notify(br->dev, mp, p);
         }
  out:
         spin_unlock_bh(&br->multicast_lock);
-err:
         kfree(priv);
  }

Thanks,
Joseph