Re: [PATCH net v7 2/6] amt: fix use-after-free when a Membership Update re-arms a freed tunnel

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] This patch adds a parking stage for group nodes before they
  are freed, and nothing enforces a limit on how many can be parked.

--

amt: fix use-after-free when a Membership Update re-arms a freed tunnel

This patch reworks tunnel, group and source lifetime handling in the AMT
relay to close several use-after-free and double free races. Tunnel expiry
now re-arms itself from a last_seen timestamp, and nodes are marked dead
under tunnel->lock. Deleted groups and sources are parked on global GC
lists, and once AMT_GC_KICK (2048) nodes are parked a drain is queued
immediately.

> diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> index 3a703b79f2c9a..fc3900e255ca2 100644
> --- a/drivers/net/amt.c
> +++ b/drivers/net/amt.c

[ ... ]

> @@ -106,6 +120,62 @@ static void amt_source_gc_work(struct work_struct *work)
>  	spin_unlock_bh(&source_gc_lock);
>  }
>  
> +static void __amt_group_gc_work(void)
> +{

[ ... ]

> +	hlist_for_each_entry_safe(gnode, t, &gc_list, gc_node) {
> +		cancel_delayed_work_sync(&gnode->group_timer);
> +		hlist_del_init(&gnode->gc_node);
> +		kfree_rcu(gnode, rcu);
> +		cond_resched();
> +	}
> +}

[ ... ]

> +static void amt_gc_parked(void)
> +{
> +	if (++amt_gc_queued == AMT_GC_KICK)
> +		mod_delayed_work(amt_wq, &group_gc_wq, 0);
> +}

[ ... ]

> @@ -266,10 +347,11 @@ static void amt_del_group(struct amt_dev *amt, struct amt_group_node *gnode)
>  		hlist_for_each_entry_safe(snode, t, &gnode->sources[i], node)
>  			amt_destroy_source(snode);
>  
> -	/* tunnel->lock was acquired outside of amt_del_group()
> -	 * But rcu_read_lock() was acquired too so It's safe.
> -	 */
> -	kfree_rcu(gnode, rcu);
> +	WRITE_ONCE(gnode->tunnel_list, NULL);
> +	spin_lock_bh(&source_gc_lock);
> +	hlist_add_head(&gnode->gc_node, &group_gc_list);
> +	amt_gc_parked();
> +	spin_unlock_bh(&source_gc_lock);
>  }

[Severity: Medium]
Is there anything that actually limits how many groups end up on
group_gc_list?

Before this patch, amt_del_group() passed the group straight to
kfree_rcu(). Now it decrements tunnel->nr_groups and parks the group on
the module-global group_gc_list. Each parked group is sizeof(*gnode) plus
256 hlist_heads, so a kmalloc-4k object.

amt_add_group() only checks the per-tunnel count:

	if (tunnel->nr_groups >= amt->max_groups)
		return ERR_PTR(-ENOSPC);

nr_groups is decremented before the group is parked. That means an
authenticated gateway sending IGMPv2 Report/Leave pairs, or the MLDv1
equivalents, parks one group per pair and never reaches max_groups:

  amt_igmpv2_report_handler()->amt_add_group()
  amt_igmpv2_leave_handler()->amt_del_group()

amt_gc_parked() never refuses or throttles parking. It queues the drain
once, at the moment the counter equals AMT_GC_KICK. A single work item does
the draining, amt_group_gc_work()->__amt_group_gc_work(), with one
cancel_delayed_work_sync(), kfree_rcu() and cond_resched() per node. With
several tunnels (different source ports or addresses), groups can be
parked from softirq on many CPUs at once.

Can the backlog keep growing if parking outpaces that one worker? Two
examples would be RSS spreading Leave or Done floods across CPUs, or
softirq load starving the unbound kworker. The per-CPU kfree_rcu() batching
used before had a shrinker. This list has no shrinker and no high-water
mark.

The commit message says:

  the kick keeps the backlog near that instead of letting it grow for
  up to 30 seconds

Is that enforced anywhere, or does it rely on the drain keeping up?

The lists and counters are also shared by every AMT device in every
network namespace. Churn on one device grows the backlog for all of them,
and it also lengthens the __amt_group_gc_work() drains that amt_dev_stop()
runs under RTNL for other devices.

Would a hard cap on amt_gc_queued, checked in amt_add_group() where groups
are admitted, fit better here?

Later patches in the series don't seem to change this either.
amt_gc_parked(), amt_del_group() and amt_add_group() are still the same in
"amt: create relay tunnels only from authenticated Updates".

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110210538.217936-1-omar%40blockcast.net