This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: bridge: mcast: don't truncate the port group walk on teardown
This changes br_multicast_del_pg() to unlink the port group from
port->mglist with hlist_del_init_rcu() instead of hlist_del_init(), so
that a nested deletion of another group of the same port does not clear
the ->next pointer that an in-progress teardown walk has already latched.
The intent is to stop br_multicast_del_port() and
__br_multicast_disable_port_ctx() from terminating early and leaving port
groups behind with a dangling ->key.port.
> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> index 75e1e2a8fc83a..452ac68c4a835 100644
> --- a/net/bridge/br_multicast.c
> +++ b/net/bridge/br_multicast.c
> @@ -808,7 +808,11 @@ void br_multicast_del_pg(struct net_bridge_mdb_entry *mp,
> struct hlist_node *tmp;
>
> rcu_assign_pointer(*pp, pg->next);
> - hlist_del_init(&pg->mglist);
> + /* Keep ->next (held under multicast_lock, freed later by the GC work):
> + * a port->mglist teardown walk may have latched this node as its next,
> + * and deleting other groups of the same port must not truncate it.
> + */
> + hlist_del_init_rcu(&pg->mglist);
[Severity: High]
Preserving ->next keeps the walk going, but doesn't it now make the walk
visit a group that was already deleted as a side effect?
hlist_del_init_rcu() only clears ->pprev, so the latched next node is
still reachable by the iterator:
include/linux/rculist.h:
static inline void hlist_del_init_rcu(struct hlist_node *n)
{
if (!hlist_unhashed(n)) {
__hlist_del(n);
WRITE_ONCE(n->pprev, NULL);
}
}
Taking br_multicast_del_port() as the example, all under
br->multicast_lock:
br_multicast_del_port()
hlist_for_each_entry_safe(pg, n, &port->mglist, mglist) /* n = A->next = B
*/
br_multicast_find_del_pg(br, A)
br_multicast_del_pg(mp, A, pp)
br_multicast_del_group_src() -> br_multicast_fwd_src_remove()
or br_multicast_star_g_handle_mode() -> __fwd_del_star_excl()
or br_multicast_sg_del_exclude_ports()
-> deletes B (same port) via br_multicast_del_pg()
rcu_assign_pointer(*pp, B->next); /* B off mp->ports */
hlist_del_init_rcu(&B->mglist); /* B->next preserved */
pg = n = B /* loop continues into B
*/
br_multicast_find_del_pg(br, B)
In br_multicast_find_del_pg() the mdb entry lookup still succeeds, since
mdb entries are only unhashed under br->multicast_lock, but the scan over
mp->ports can no longer find B because br_multicast_del_pg() already did
rcu_assign_pointer(*pp, pg->next) for it:
for (pp = &mp->ports;
(p = mlock_dereference(*pp, br)) != NULL;
pp = &p->next) {
if (p != pg)
continue;
br_multicast_del_pg(mp, pg, pp);
return;
}
WARN_ON(1);
So control falls through to the trailing WARN_ON(1). br_multicast_del_port()
applies no filter at all before calling br_multicast_find_del_pg(), and the
filter in __br_multicast_disable_port_ctx():
hlist_for_each_entry_safe(pg, n, &pmctx->port->mglist, mglist)
if (!(pg->flags & MDB_PG_FLAGS_PERMANENT) &&
(!br_multicast_port_ctx_is_vlan(pmctx) ||
pg->key.addr.vid == pmctx->vlan->vid))
br_multicast_find_del_pg(pmctx->port->br, pg);
doesn't exclude a kernel-installed, non-permanent S,G group either.
Does that mean the reproducer described in the commit message now produces
a WARN backtrace on the same path instead of the KASAN use-after-free, and
a panic when booted with panic_on_warn=1?
Would it make sense to also skip nodes that are already unlinked in both
teardown walks, e.g.:
hlist_for_each_entry_safe(pg, n, &port->mglist, mglist) {
if (hlist_unhashed(&pg->mglist))
continue;
br_multicast_find_del_pg(br, pg);
}
or otherwise restart the walk, rather than relying on the preserved ->next
alone?
[Severity: High]
This isn't a bug introduced by this patch, but the same list has a second
deletion site that still uses the non-RCU helper and frees immediately,
br_multicast_del_port_group():
void br_multicast_del_port_group(struct net_bridge_port_group *p)
{
struct net_bridge_port *port = p->key.port;
__u16 vid = p->key.addr.vid;
hlist_del_init(&p->mglist);
if (!br_multicast_is_star_g(&p->key.addr))
rhashtable_remove_fast(&port->br->sg_port_tbl,
&p->rhnode,
br_sg_port_rht_params);
kfree(p);
br_multicast_port_ngroups_dec(port, vid);
}
port->mglist is also walked locklessly by br_multicast_list_adjacent(),
which only holds rcu_read_lock():
hlist_for_each_entry_rcu(group, &port->mglist, mglist) {
...
entry->addr = group->key.addr;
That reader is exported and used from net/batman-adv/multicast.c. The group
is already published on port->mglist by br_multicast_new_port_group()
(hlist_add_head(&p->mglist, &port->mglist)) before the only caller of
br_multicast_del_port_group() runs, the err_del_port_group label in
br_mdb_add_group_star_g() in net/bridge/br_mdb.c, reached when
br_mdb_add_group_srcs() fails. br_mdb_config_src_list_init() does not
reject a duplicated source, so a MDBE_ATTR_SRC_LIST carrying the same
source twice without NLM_F_REPLACE makes br_mdb_add_group_src() return
-EEXIST; an allocation failure returning -ENOSPC is a second trigger.
spin_lock_bh(&br->multicast_lock) held by the deleting side does not
exclude an RCU reader on another CPU. Can br_multicast_list_adjacent()
then read group->key.addr and follow group->mglist.next after the plain
kfree(p), and does the hlist_del_init() there also truncate that RCU walk
by NULLing ->next?
Since this patch establishes that port->mglist deletions must preserve
->next and defer the free, would it be worth converting this site to
hlist_del_init_rcu() plus a deferred free, in the way
br_multicast_destroy_port_group() uses kfree_rcu()?
