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

[Paolo Abeni <pabeni@redhat.com>]

On 9/2/26 2:54 PM, Nikolay Aleksandrov wrote:
> 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.
I understand your frustration. Unfortunately ignoring this kind of
reports will lead to at least one security report, possibly with
unbelievably high impact due to unexpected chaining. That is, we can't
ignore them.

/P