On 03/09/2026 12:12, Paolo Abeni wrote:
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, [email protected] 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
Sure, I know. :) Sorry about the noise, I guess frustration got the best of me.
I've
already prepared the proper fix that converts mglist to use RCU and we don't
need
the spinlock at all. I'll send it out after running a few tests.
Cheers,
Nik
