* [PATCH net v2] net: bridge: mcast: fix br_multicast_list_adjacent rcu walk of mglist
@ 2026-09-01 7:40 Nikolay Aleksandrov
2026-09-02 12:43 ` [net,v2] " netdev-bot+sashiko
0 siblings, 1 reply; 6+ messages in thread
From: Nikolay Aleksandrov @ 2026-09-01 7:40 UTC (permalink / raw)
To: netdev
Cc: idosch, davem, edumazet, kuba, pabeni, horms, linus.luessing,
bridge, Nikolay Aleksandrov
Sashiko reported a bug [1] that br_multicast_del_port_group unlists the
port group not using proper rcu helper that preserves the next pointer and
after that immediately frees the port group without waiting for rcu grace
period. The only rcu walker of mglist is br_multicast_list_adjacent() and
it turns out that function has always been buggy because mglist was never
converted to RCU. Fix it by acquiring the bridge's multicast lock for the
mglist walk. Return -ENOMEM on allocation error.
[1] https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260826014200.362304-1-littleddfu%40gmail.com
Reviewed-by: Ido Schimmel <idosch@nvidia.com>
Fixes: 07f8ac4a1e26 ("bridge: add export of multicast database adjacent to net_dev")
Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
v2: while changing this fn, return -ENOMEM on error and document it in
the kdoc (sashiko)
net/bridge/br_multicast.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 3ef5d8bbf552..97686984de6d 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -4936,13 +4936,15 @@ void br_multicast_set_startup_query_intvl(struct net_bridge_mcast *brmctx,
* snooping feature on all bridge ports of dev's bridge device, excluding
* the addresses from dev itself.
*
- * Returns the number of items added to br_ip_list.
+ * Return: The number of items added to br_ip_list or -ENOMEM on memory
+ * allocation error
*
* Notes:
* - br_ip_list needs to be initialized by caller
* - br_ip_list might contain duplicates in the end
* (needs to be taken care of by caller)
* - br_ip_list needs to be freed by caller
+ * - on -ENOMEM the caller must free any allocated entries
*/
int br_multicast_list_adjacent(struct net_device *dev,
struct list_head *br_ip_list)
@@ -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);
}
unlock:
--
2.47.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [net,v2] net: bridge: mcast: fix br_multicast_list_adjacent rcu walk of mglist
2026-09-01 7:40 [PATCH net v2] net: bridge: mcast: fix br_multicast_list_adjacent rcu walk of mglist Nikolay Aleksandrov
@ 2026-09-02 12:43 ` netdev-bot+sashiko
2026-09-02 12:51 ` Nikolay Aleksandrov
0 siblings, 1 reply; 6+ messages in thread
From: netdev-bot+sashiko @ 2026-09-02 12:43 UTC (permalink / raw)
To: razor
Cc: netdev, idosch, davem, edumazet, kuba, pabeni, horms,
linus.luessing, bridge
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [net,v2] net: bridge: mcast: fix br_multicast_list_adjacent rcu walk of mglist
2026-09-02 12:43 ` [net,v2] " netdev-bot+sashiko
@ 2026-09-02 12:51 ` Nikolay Aleksandrov
2026-09-02 12:54 ` Nikolay Aleksandrov
0 siblings, 1 reply; 6+ messages in thread
From: Nikolay Aleksandrov @ 2026-09-02 12:51 UTC (permalink / raw)
To: netdev-bot+sashiko
Cc: netdev, idosch, davem, edumazet, kuba, pabeni, horms,
linus.luessing, bridge
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [net,v2] net: bridge: mcast: fix br_multicast_list_adjacent rcu walk of mglist
2026-09-02 12:51 ` Nikolay Aleksandrov
@ 2026-09-02 12:54 ` Nikolay Aleksandrov
2026-09-03 9:12 ` Paolo Abeni
0 siblings, 1 reply; 6+ messages in thread
From: Nikolay Aleksandrov @ 2026-09-02 12:54 UTC (permalink / raw)
To: netdev-bot+sashiko
Cc: netdev, idosch, davem, edumazet, kuba, pabeni, horms,
linus.luessing, bridge
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [net,v2] net: bridge: mcast: fix br_multicast_list_adjacent rcu walk of mglist
2026-09-02 12:54 ` Nikolay Aleksandrov
@ 2026-09-03 9:12 ` Paolo Abeni
2026-09-03 9:15 ` Nikolay Aleksandrov
0 siblings, 1 reply; 6+ messages in thread
From: Paolo Abeni @ 2026-09-03 9:12 UTC (permalink / raw)
To: Nikolay Aleksandrov, netdev-bot+sashiko
Cc: netdev, idosch, davem, edumazet, kuba, horms, linus.luessing,
bridge
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [net,v2] net: bridge: mcast: fix br_multicast_list_adjacent rcu walk of mglist
2026-09-03 9:12 ` Paolo Abeni
@ 2026-09-03 9:15 ` Nikolay Aleksandrov
0 siblings, 0 replies; 6+ messages in thread
From: Nikolay Aleksandrov @ 2026-09-03 9:15 UTC (permalink / raw)
To: Paolo Abeni, netdev-bot+sashiko
Cc: netdev, idosch, davem, edumazet, kuba, horms, linus.luessing,
bridge
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, 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
>
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-09-03 9:15 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-01 7:40 [PATCH net v2] net: bridge: mcast: fix br_multicast_list_adjacent rcu walk of mglist Nikolay Aleksandrov
2026-09-02 12:43 ` [net,v2] " netdev-bot+sashiko
2026-09-02 12:51 ` Nikolay Aleksandrov
2026-09-02 12:54 ` Nikolay Aleksandrov
2026-09-03 9:12 ` Paolo Abeni
2026-09-03 9:15 ` Nikolay Aleksandrov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox