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
