Re: [PATCH net v3 6/6] amt: do not create tunnel state for unauthenticated Requests

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 (3) · Medium: 3 · Low: 1

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 patch says three secret generations keep a response MAC
  valid "for 120-180s, at least the amt_gmi() the Query advertised".
- [Medium] amt_request_handler() still sends
  icmp_ndo_send(ICMP_DEST_UNREACH, ICMP_HOST_UNREACH) to the claimed,…
- [Medium] The patch withholds the Query when a later verified Update
  could not be admitted.
- [Low] The comment on AMT_MAX_TUNNELS_PER_SRC in include/net/amt.h says
  deployments needing more than 16 tunnels behind one address "use the…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it remains on
  the path the patch rewrites.
- [High] This problem wasn't introduced by this patch, but tunnel
  teardown can free a group and its tunnel while amt_source_work() is…
- [High] This problem wasn't introduced by this patch, but the patch
  moves tunnel allocation to a site where it stays open.

--

amt: do not create tunnel state for unauthenticated Requests

This patch stops amt_request_handler() from allocating tunnel state for
unauthenticated Membership Requests. amt_update_handler() now recomputes
the keyed MAC from packet fields against three rotating secrets. Tunnel
state is created only after an Update verifies, in the new
amt_tunnel_get_or_create(), which also enforces a fixed per-source cap.

> diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> index fec32650cb91f..2d9f2d95bd959 100644
> --- a/drivers/net/amt.c
> +++ b/drivers/net/amt.c

[ ... ]

> @@ -925,7 +934,13 @@ static void amt_secret_work(struct work_struct *work)
>  					   secret_wq);
>  
>  	spin_lock_bh(&amt->lock);
> -	get_random_bytes(&amt->key, sizeof(siphash_key_t));
> +	/* Rotate: keep the two previous generations so a MAC handed out just
> +	 * before a rotation still verifies for up to three AMT_SECRET_TIMEOUT
> +	 * windows (120-180s), covering the amt_gmi() the Query advertised.
> +	 */
> +	amt->key[2] = amt->key[1];
> +	amt->key[1] = amt->key[0];
> +	get_random_bytes(&amt->key[0], sizeof(siphash_key_t));

[Severity: Medium]
Do three generations actually cover amt_gmi()?

AMT_SECRET_TIMEOUT is 60000 ms and only key[0..2] are kept. A MAC minted
just before a rotation therefore verifies for as little as 120s, and at
most 180s.

With the relay defaults from amt_newlink() (qrv = 2, qi = 125, qri = 10),
amt_gmi() is ((2 * 125) + 10) * 1000 = 260s. The commit message quotes
that figure itself.

Even the 120s floor is shorter than the 125s QQIC that amt_build_igmp_gq()
advertises:

    ihv3->qqic	= amt->qi;

The commit message says:

    Three generations keep a MAC valid for 120-180s, at least the
    amt_gmi() the Query advertised; two generations (120s floor) could
    expire a MAC before the interval it promised.

The same claim is repeated in the new amt_update_handler() comment. Also,
two generations would give a 60s floor, not 120s.

Before this patch, tunnel->mac was stored and stayed valid until the
gateway's next Request replaced it. Take a gateway that refreshes its
Request at the advertised 125s QQIC and got its MAC just before a
rotation. Won't its Updates in the last few seconds of each interval be
dropped in amt_update_handler() as "Invalid MAC", even on an established
tunnel?

The Linux gateway re-requests often enough to avoid this. Third-party
gateways that pace Requests by QQIC would not.

>  	spin_unlock_bh(&amt->lock);

[ ... ]

> @@ -2490,14 +2511,99 @@ static bool amt_membership_query_handler(struct amt_dev *amt,

[ ... ]

> +static struct amt_tunnel_list *amt_tunnel_get_or_create(struct amt_dev *amt,
> +							__be32 saddr,
> +							__be16 sport)
> +{
> +	struct amt_tunnel_list *tunnel;
> +	int per_src = 0;
> +	int i;
> +
> +	tunnel = amt_tunnel_lookup(amt, saddr, sport);
> +	if (tunnel)
> +		return tunnel;

[Severity: High]
This isn't a bug introduced by this patch, but this lockless lookup can
return a tunnel that amt_tunnel_expire() has already marked dead. Does
anything stop the Update path from adding groups to it?

amt_update_handler() takes tunnel->lock and calls
amt_igmp_report_handler() or amt_mld_report_handler(). These can call
amt_add_group(), which stores the tunnel pointer in gnode->tunnel_list and
arms gnode->group_timer with dev_hold(). Nothing on that path checks
tunnel->dead.

A possible interleaving:

CPU1 (amt_update_handler)            CPU2 (amt_tunnel_expire)
amt_tunnel_lookup() finds tunnel
                                     reads stale last_seen
                                     tunnel->dead = true;
                                     list_del_rcu(&tunnel->list);
                                     amt_clear_groups(tunnel);
WRITE_ONCE(tunnel->last_seen, ...);
spin_lock_bh(&tunnel->lock);
amt_igmp_report_handler()
    amt_add_group()   /* new gnode on dying tunnel */
                                     kfree_rcu(tunnel, rcu);

When the orphaned group_timer fires later:

amt_group_work()
    struct amt_tunnel_list *tunnel = gnode->tunnel_list;
    ...
    spin_lock_bh(&tunnel->lock);    /* tunnel already freed */

The same window exists for Updates still in flight while amt_dev_stop()
runs amt_clear_groups().

The comment above WRITE_ONCE(tunnel->last_seen, jiffies) says this "cannot
re-queue the work of a tunnel that is being freed". That holds for gc_wq,
but not for group_timer.

Would checking READ_ONCE(tunnel->dead) under tunnel->lock in the Update
path, and bailing out, close this? ->dead is set before amt_clear_groups()
takes tunnel->lock.

> +
> +	spin_lock_bh(&amt->lock);

[ ... ]

> +	if (amt->nr_tunnels >= amt->max_tunnels ||
> +	    per_src >= AMT_MAX_TUNNELS_PER_SRC) {
> +		spin_unlock_bh(&amt->lock);
> +		return NULL;
> +	}

[Severity: High]
This is a pre-existing issue, but the patch moves tunnel allocation here.
Neither amt_update_handler() nor amt_tunnel_get_or_create() checks whether
the device is running or stopping. Can a tunnel be created after
amt_dev_stop() has drained the list?

amt_dev_stop() does:

    RCU_INIT_POINTER(amt->sk, NULL);
    synchronize_net();
    if (sk)
        udp_tunnel_sock_release(sk);
    ...
    spin_lock_bh(&amt->lock);
    list_for_each_entry(tunnel, &amt->tunnel_list, list)
        tunnel->dead = true;
    list_splice_init(&amt->tunnel_list, &doomed);
    amt->nr_tunnels = 0;
    spin_unlock_bh(&amt->lock);

udp_tunnel_sock_release() clears sk_user_data but doesn't wait for
amt_rcv() callers that already fetched amt. A verified Update received
between synchronize_net() and that clear can get here after the splice.
It would then list_add_tail_rcu() a new tunnel and arm its gc_wq for
3 * amt_gmi().

This stop never tears that tunnel down. amt_dev_uninit() and amt_dellink()
do no tunnel cleanup, and needs_free_netdev is set. After the device is
deleted, amt_tunnel_expire() would do spin_lock_bh(&tunnel->amt->lock) on
a freed amt_dev.

Separately, list_splice_init() is not RCU-safe on the RCU-traversed
tunnel_list. A concurrent amt_tunnel_lookup() or amt_dev_xmit() reader at
the last entry can follow ->next to the on-stack doomed head. The splice
came from an earlier commit in this series.

__LINK_STATE_START is cleared before ndo_stop. Would a netif_running()
check at the start of amt_update_handler(), or under amt->lock here, close
the creation race?

[ ... ]

> +	/* Publish only once the entry is fully initialised. */
> +	list_add_tail_rcu(&tunnel->list, &amt->tunnel_list);
> +	amt->nr_tunnels++;
> +	mod_delayed_work(amt_wq, &tunnel->gc_wq,
> +			 msecs_to_jiffies(amt_gmi(amt)) * 3);
> +	spin_unlock_bh(&amt->lock);

[Severity: High]
This isn't a bug introduced by this patch. Tunnels created here are torn
down later by amt_tunnel_expire() or amt_dev_stop(). Can that teardown
free a group and its tunnel while amt_source_work() is still running for
one of the group's sources?

amt_source_work() reads the parent pointer before it takes any lock or
RCU read lock:

    tunnel = gnode->tunnel_list;
    spin_lock_bh(&tunnel->lock);
    rcu_read_lock();

The teardown path is amt_clear_groups()->amt_del_group()->
amt_destroy_source(), and amt_destroy_source() only cancels
asynchronously:

    cancel_delayed_work(&snode->source_timer);
    hlist_del_init_rcu(&snode->node);

kfree_rcu(gnode, rcu) and kfree_rcu(tunnel, rcu) are then queued while
the source work may still be running.

With CONFIG_PREEMPT, the worker can be preempted between reading
gnode->tunnel_list and spin_lock_bh(). It is not in an RCU reader at that
point, so the grace period can complete and spin_lock_bh(&tunnel->lock)
runs on freed memory.

Without preemption, the worker can be spinning on tunnel->lock while
amt_clear_groups() holds it. It then calls amt_destroy_source() a second
time on an snode that is already on source_gc_list. hlist_del_init_rcu()
unlinks it from source_gc_list without source_gc_lock, racing
__amt_source_gc_work(), and nr_sources is decremented twice.

amt_dev_stop() only syncs gc_wq and never drains these source works.

[ ... ]

> @@ -2740,67 +2882,42 @@ static bool amt_request_handler(struct amt_dev *amt, struct sk_buff *skb)

[ ... ]

> +	spin_lock_bh(&amt->lock);
> +	key = amt->key[0];
> +	full = amt->nr_tunnels >= amt->max_tunnels;
>  	spin_unlock_bh(&amt->lock);

[ ... ]

> +	/* When the table is full, refuse a source that holds no tunnel without
> +	 * sending a Query.  A silent Query would keep the gateway believing the
> +	 * exchange is progressing while its later, verified Update is refused;
> +	 * withholding it lets the gateway's req_cnt climb and drive rediscovery
> +	 * instead.  The ICMP is rate-limited and no state is allocated, so this
> +	 * is not the reflector the pre-verification allocation path used to be.
> +	 * A source that holds a tunnel is answered: its periodic Request is how
> +	 * it renews the response MAC its Updates echo.
>  	 */

[Severity: Medium]
Should the full check above also account for AMT_MAX_TUNNELS_PER_SRC?

It only looks at the global limit. amt_tunnel_get_or_create() also refuses
when per_src >= AMT_MAX_TUNNELS_PER_SRC.

Suppose a source address already holds 16 tunnels and sends a Request
from a new port, for example another gateway behind the same NAT address:

  amt_request_handler() answers with a Query
  gateway: amt_membership_query_handler() sets ready4/ready6, req_cnt = 0
  amt_update_handler()->amt_tunnel_get_or_create() returns NULL
  amt_update_handler() sends ICMP host unreachable
  gateway: amt_err_lookup() does mod_delayed_work(amt_wq, &amt->req_wq, 0)
  next Request is answered with another Query, req_cnt reset again

Doesn't this loop indefinitely? It is the "silent Query" case this comment
says it avoids, and the gateway never reaches rediscovery.

[ ... ]

> -	if (!netif_running(amt->dev) || !netif_running(amt->stream_dev))
> +	if (full && !amt_tunnel_lookup(amt, iph->saddr, udph->source)) {
> +		icmp_ndo_send(skb, ICMP_DEST_UNREACH, ICMP_HOST_UNREACH, 0);
>  		return true;
> +	}

[Severity: Medium]
Is this still an ICMP reflector toward spoofed sources?

A spoofed victim address never holds a tunnel. So every spoofed Request
that arrives while the table is full still sends ICMP_DEST_UNREACH to its
claimed source, using the same icmp_ndo_send() call as the old full-table
path.

Filling the table now takes verified tunnels. With AMT_MAX_TUNNELS_PER_SRC
at 16, that is 128 / 16 = 8 real IP addresses, or just a table that is
legitimately full of real gateways.

The comment says "is not the reflector the pre-verification allocation
path used to be". The commit message says "The ICMP is rate-limited and
allocates nothing, unlike the old reflector".

The old full-table path also returned before kzalloc() and had the same
icmp_global/inet_peer rate limiting. Also, the ICMP path can allocate: for
each new spoofed destination,
__icmp_send()->icmpv4_xrlim_allow()->inet_getpeer_v4() can create an
inet_peer.

Each spoofed Request also uses up part of the host-wide icmp_msgs_per_sec
budget.

Are these claims in the comment and commit message accurate?

[ ... ]

> diff --git a/include/net/amt.h b/include/net/amt.h
> index 0520121ded649..1c7c144e4db31 100644
> --- a/include/net/amt.h
> +++ b/include/net/amt.h

[ ... ]

> @@ -394,6 +393,12 @@ struct amt_dev {
>  #define AMT_SECRET_TIMEOUT	60000
>  #define IANA_AMT_UDP_PORT	2268
>  #define AMT_MAX_TUNNELS         128
> +/* Per-source tunnel cap.  A single gateway needs at most one tunnel per
> + * family; this stops one verified source endpoint from monopolising the
> + * AMT_MAX_TUNNELS-slot table.  Deployments that legitimately need more behind
> + * one address (e.g. CGNAT) use the configurable net-next per-source bound.
> + */
> +#define AMT_MAX_TUNNELS_PER_SRC 16

[Severity: Low]
Which configurable per-source bound does this comment refer to?

This tree exposes only IFLA_AMT_MAX_TUNNELS in include/uapi/linux/amt.h,
and there is no module parameter. The check in amt_tunnel_get_or_create()
is unconditional, so a relay with more than 16 gateways behind one public
address has no way to raise it.

This patch carries a Fixes: tag and is described as backportable. Should
the comment avoid pointing stable users at a mechanism that does not exist
here?

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