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

[Nikolay Aleksandrov <razor@blackwall.org>]

On 02/09/2026 15:51, Nikolay Aleksandrov wrote:
> On 02/09/2026 15:43, netdev-bot+sashiko@kernel.org wrote:
>> 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?
>>
> 
> Oh please, enough with this shit. This is taking way too much time for a function
> that is largely useless.
> 
> I will just convert it properly to RCU in -net and be done with this.
> 

and just to be clear - IMO only bots and llm "security" researchers would be trying
to do what is suggested above, I doubt there is any real-world case where it would
actually happen.