Re: [net,v2] net: bridge: mcast: fix br_multicast_list_adjacent rcu walk of mglist

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] net/bridge/br_multicast.c:br_multicast_list_adjacent() now
  performs the entire per-port mglist walk, including one…

--

net: bridge: mcast: fix br_multicast_list_adjacent rcu walk of mglist

The patch converts the mglist walk in br_multicast_list_adjacent() from an
RCU walk to one done under the bridge's multicast_lock, since mglist was
never converted to RCU.  It also makes the function return -ENOMEM on
allocation failure and documents that in the kdoc.

Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260826014200.362304-1-littleddfu%40gmail.com

> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> index 3ef5d8bbf552e..97686984de6d5 100644
> --- a/net/bridge/br_multicast.c
> +++ b/net/bridge/br_multicast.c

[ ... ]

> @@ -4967,15 +4969,20 @@ int br_multicast_list_adjacent(struct net_device *dev,
>  		if (!port->dev || port->dev == dev)
>  			continue;
>  
> -		hlist_for_each_entry_rcu(group, &port->mglist, mglist) {
> +		spin_lock_bh(&br->multicast_lock);
> +		hlist_for_each_entry(group, &port->mglist, mglist) {
>  			entry = kmalloc_obj(*entry, GFP_ATOMIC);
> -			if (!entry)
> +			if (!entry) {
> +				spin_unlock_bh(&br->multicast_lock);
> +				count = -ENOMEM;
>  				goto unlock;
> +			}
>  
>  			entry->addr = group->key.addr;
>  			list_add(&entry->list, br_ip_list);
>  			count++;
>  		}
> +		spin_unlock_bh(&br->multicast_lock);
>  	}

[Severity: Medium]
How long can this critical section get?  The whole per-port mglist walk,
including one kmalloc_obj(*entry, GFP_ATOMIC) per port group, now runs with
br->multicast_lock held and bottom halves disabled.

The number of allocations inside the section equals the length of
port->mglist, which is only bounded by br->hash_max in
br_multicast_new_group():

net/bridge/br_multicast.c:br_multicast_new_group() {
	...
	if (atomic_read(&br->mdb_hash_tbl.nelems) >= br->hash_max) {
	...
}

hash_max defaults to BR_MULTICAST_DEFAULT_HASH_MAX (4096) and can be set to
any u32 without validation via netlink
(br->hash_max = nla_get_u32(data[IFLA_BR_MCAST_HASH_MAX]) in br_netlink.c)
or via set_hash_max() in br_sysfs_br.c.  The list contents themselves are
populated by IGMP/MLD reports from hosts on the bridged L2 domain.

While the lock is held, every other CPU that needs br->multicast_lock spins
for it, including the br_multicast_rcv() receive path, the group and query
expiry timers and br_multicast_port_group_rexmit().  Before this change the
loop only held rcu_read_lock(), so a long mglist slowed the caller but did
not block bridge multicast processing.

The caller is periodic, so this is not a one-off:

batadv_mcast_mla_update()
  __batadv_mcast_mla_update()
    batadv_mcast_mla_bridge_get()
      br_multicast_list_adjacent()

which runs every BATADV_MCAST_WORK_PERIOD (500 ms) from the batman-adv
workqueue.

Would it be preferable to bound the section, for example by pre-allocating
the entries outside the lock, or by dropping and re-taking the lock in
batches, rather than holding a bh-disabling spinlock across up to hash_max
slab allocations?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260901074046.316190-1-razor%40blackwall.org