Re: [PATCH net-next v2] amt: make the per-tunnel group and source limits configurable

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 (1) · 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] The new AMT_MAX_SOURCE_LIMIT ceiling (4096) multiplies the
  per-packet softirq CPU cost of an existing O(N×M) loop by up to 32x.

Pre-existing issues:
- [High] Teardown race between source-timer work and group/tunnel
  deletion causes double-destroy and use-after-free (this problem wasn't…

--

amt: make the per-tunnel group and source limits configurable

This patch adds IFLA_AMT_MAX_GROUPS and IFLA_AMT_MAX_SOURCES, so a relay
operator can raise the per-tunnel group and source record limits when the
link is created. The netlink policy caps them at 1024 groups and 4096
sources, and amt_fill_info() reports both values.

> diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> index 81d26ef82cf5c..30cff8de4830f 100644
> --- a/drivers/net/amt.c
> +++ b/drivers/net/amt.c

[ ... ]

> @@ -3170,6 +3171,8 @@ static const struct nla_policy amt_policy[IFLA_AMT_MAX + 1] = {
>  	[IFLA_AMT_REMOTE_IP]	= { .len = sizeof_field(struct iphdr, daddr) },
>  	[IFLA_AMT_DISCOVERY_IP]	= { .len = sizeof_field(struct iphdr, daddr) },
>  	[IFLA_AMT_MAX_TUNNELS]	= { .type = NLA_U32 },
> +	[IFLA_AMT_MAX_GROUPS]	= NLA_POLICY_MAX(NLA_U32, AMT_MAX_GROUP_LIMIT),
> +	[IFLA_AMT_MAX_SOURCES]	= NLA_POLICY_MAX(NLA_U32, AMT_MAX_SOURCE_LIMIT),

[Severity: Medium]
The 4096 ceiling is justified by memory cost. The include/net/amt.h
comment says "Gateways drive these allocations, so keep one tunnel's worst
case to a few MiB". Could this ceiling also raise the per-packet softirq
cost of amt_lookup_act_srcs() by up to 32x?

amt_add_srcs() only checks the tunnel-wide count:

	for (i = 0; i < nsrcs; i++) {
		if (tunnel->nr_sources >= amt->max_sources)
			return;

So all max_sources records can sit in a single group. The AMT_OPS_SUB
branch of amt_lookup_act_srcs() then walks every source in that group and
compares each one against every source in the incoming record:

	case AMT_OPS_SUB:
		/* A-B */
		for (i = 0; i < amt->hash_buckets; i++) {
			hlist_for_each_entry_safe(snode, t, &gnode->sources[i],
						  node) {
				if (!amt_status_filter(snode, filter))
					continue;
				for (j = 0; j < nsrcs; j++) {

This loop runs from amt_update_handler()->amt_mcast_is_in_handler() and
from the EXCLUDE-mode handler. It runs in softirq with
spin_lock_bh(&tunnel->lock) held.

In amt_igmpv3_report_handler(), only ip_mc_may_pull() bounds nsrcs. A
reassembled datagram can therefore carry about 16K IPv4 sources in one
record. That is about 2M address compares per record at 128 sources, and
about 67M at 4096, all with BHs disabled.

Can a remote gateway that has completed the request/query exchange use
this to cause soft lockups or RCU stalls on a relay that raised
IFLA_AMT_MAX_SOURCES?

One step is not confirmed: whether a gateway can keep thousands of one
group's sources in D_FWD/OLD state, so that they pass
amt_status_filter().

>  };
>  
>  static int amt_validate(struct nlattr *tb[], struct nlattr *data[],
> @@ -3235,8 +3238,10 @@ static int amt_newlink(struct net_device *dev,
>  		amt->max_tunnels = AMT_MAX_TUNNELS;
>  
>  	spin_lock_init(&amt->lock);
> -	amt->max_groups = AMT_MAX_GROUP;
> -	amt->max_sources = AMT_MAX_SOURCE;
> +	amt->max_groups = nla_get_u32_default(data[IFLA_AMT_MAX_GROUPS], 0) ?:
> +			  AMT_MAX_GROUP;
> +	amt->max_sources = nla_get_u32_default(data[IFLA_AMT_MAX_SOURCES], 0) ?:
> +			   AMT_MAX_SOURCE;

[Severity: High]
This is a pre-existing issue and was not introduced by this patch. This
patch lets a tunnel hold more groups and sources, though. Is source timer
teardown in amt_destroy_source() safe against an amt_source_work() that is
already running?

amt_destroy_source() uses the non-synchronous cancel:

	cancel_delayed_work(&snode->source_timer);
	hlist_del_init_rcu(&snode->node);
	tunnel->nr_sources--;
	gnode->nr_sources--;
	spin_lock_bh(&source_gc_lock);
	hlist_add_head_rcu(&snode->node, &source_gc_list);
	spin_unlock_bh(&source_gc_lock);

amt_source_work() reads snode->gnode and gnode->tunnel_list before it
takes any lock or rcu_read_lock(). It also never checks whether snode has
already been unhashed:

	struct amt_group_node *gnode = snode->gnode;
	...
	tunnel = gnode->tunnel_list;
	spin_lock_bh(&tunnel->lock);
	rcu_read_lock();
	if (gnode->filter_mode == MCAST_INCLUDE) {
		amt_destroy_source(snode);

amt_wq is WQ_UNBOUND. Consider amt_source_work() spinning on tunnel->lock
on one CPU while another CPU handles an IGMPv2 leave from the gateway:

CPU B (softirq, tunnel->lock held)
amt_update_handler()
  amt_igmpv2_leave_handler()
    amt_del_group()
      amt_destroy_source(snode)    /* cancel returns false */
      kfree_rcu(gnode, rcu)

CPU A
amt_source_work()
  spin_lock_bh(&tunnel->lock)     /* acquired after CPU B */
  amt_destroy_source(snode)       /* second time */

The second amt_destroy_source() call then does three things:

- It calls hlist_del_init_rcu() on a node that is now on source_gc_list,
  without holding source_gc_lock.
- It decrements the u32 tunnel->nr_sources and gnode->nr_sources again.
  They wrap, and amt_add_srcs() then refuses every further source for
  that tunnel.
- It adds snode to source_gc_list again, so __amt_source_gc_work() can
  kfree_rcu() it twice.

With CONFIG_PREEMPT, the worker can also be preempted after it reads
gnode and tunnel but before spin_lock_bh(). The group, or the whole tunnel
through amt_tunnel_expire()->amt_clear_groups()->kfree_rcu(tunnel, rcu),
can be freed after a grace period. When the worker resumes, it takes
tunnel->lock and reads gnode->filter_mode from freed memory.

The MLDv1 done handler, amt_group_work() and amt_tunnel_expire() reach the
same deletion paths.

>  	amt->hash_buckets = AMT_HSIZE;
>  	amt->nr_tunnels = 0;
>  	get_random_bytes(&amt->hash_seed, sizeof(amt->hash_seed));

[ ... ]

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