Re: [PATCH net v4 5/5] amt: harden relay tunnel lifetime and request admission

[netdev-bot+sashiko@kernel.org]

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

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

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] Commit "amt: make pre-query report drops visible" (in this
  baseline-to-target range) converted only amt_dev_xmit()'s counter…
- [Low] Two new comments about the per-source tunnel cap describe the
  code wrongly.
- [Low] The selftest comment above test_query_egress()
  (tools/testing/selftests/net/amt.sh:256-259) is now stale.
- [Low] amt_update_handler() now reads tunnel->dead with READ_ONCE()
  while holding tunnel->lock (drivers/net/amt.c:2696 and 2720).

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the
  group/source workers can still race with group deletion and cause a…

--

amt: harden relay tunnel lifetime and request admission

This patch makes AMT relay Membership Requests stateless. The Query MAC is
derived from the source address, port, nonce and a rotating key, and tunnel
state is only created once a Membership Update has been authenticated.
Tunnels are keyed by (address, port), four key generations are kept, new
endpoints over the global or per-source limits are dropped silently, and
tunnel->dead is rechecked under tunnel->lock in the Update path.

> diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> index 884f27fd84a4a..0124f1be673b3 100644
> --- a/drivers/net/amt.c
> +++ b/drivers/net/amt.c

[ ... ]

> @@ -797,7 +802,8 @@ static void amt_send_igmp_gq(struct amt_dev *amt,
>  		return;
>  
>  	skb_pull(skb, sizeof(struct ethhdr));
> -	if (amt_send_membership_query(amt, skb, tunnel, false)) {
> +	if (amt_send_membership_query(amt, skb, daddr, dport, nonce, mac,
> +				      false)) {
>  		amt->dev->stats.tx_dropped++;
>  		kfree_skb(skb);
>  	}

[Severity: Medium]
The previous patch in this series, "amt: make pre-query report drops
visible", changed amt_dev_xmit() to use DEV_STATS_INC() for tx_dropped and
tx_errors "so concurrent increments are not lost". Can concurrent
increments still be lost on this counter?

amt_send_igmp_gq() and amt_send_mld_gq() still do a plain read-modify-write
on the same field:

    amt->dev->stats.tx_dropped++;

In relay mode, amt_request_handler() runs from UDP softirq on one CPU, and
amt_dev_xmit() can take its free path on another CPU at the same time:

    free:
        dev_kfree_skb(skb);
        DEV_STATS_INC(dev, tx_dropped);

Nothing serialises these two paths. The plain store can therefore
overwrite the atomic increment.

The tx_errors++ sites in amt_send_discovery(), amt_send_request(),
amt_send_advertisement() and similar helpers mix the two kinds of update
in the same way.

Should these sites use DEV_STATS_INC() as well?

[ ... ]

> @@ -1115,9 +1131,16 @@ static void amt_send_multicast_data(struct amt_dev *amt,
>  			    0);
>  }
>  
> +/* Send a Membership Query to a source that has not been authenticated yet.
> + *
> + * No per-source state exists at this point and none is created: the nonce and
> + * the keyed MAC are carried in the Query and recomputed from the Membership
> + * Update when it comes back, so everything this needs is passed by value.
> + */
>  static bool amt_send_membership_query(struct amt_dev *amt,
>  				      struct sk_buff *skb,
> -				      struct amt_tunnel_list *tunnel,
> +				      __be32 daddr, __be16 dport,
> +				      __be32 nonce, u64 mac,
>  				      bool v6)

[Severity: Low]
This isn't a bug, but the comment above test_query_egress() in
tools/testing/selftests/net/amt.sh still describes the old design:

    # The relay sends its General Queries straight from the receive path, in
    # the same context that found the tunnel. A query queued on the amt device
    # instead could outlive the tunnel it was built for.

After this patch, amt_request_handler() replies to a Request without
looking up or creating a tunnel. The Query helpers now take daddr, dport,
nonce and mac by value.

Should the selftest comment be updated to match? The egress check itself
still looks useful.

[ ... ]

> @@ -2556,6 +2693,10 @@ static bool amt_update_handler(struct amt_dev *amt, struct sk_buff *skb)
>  		}
>  
>  		spin_lock_bh(&tunnel->lock);
> +		if (READ_ONCE(tunnel->dead)) {
> +			spin_unlock_bh(&tunnel->lock);
> +			return true;
> +		}
>  		amt_igmp_report_handler(amt, skb, tunnel);
>  		spin_unlock_bh(&tunnel->lock);

[Severity: Low]
This check reads tunnel->dead with READ_ONCE() while holding tunnel->lock.
Both writers use a plain store and hold only amt->lock:

amt_tunnel_expire():
    tunnel->dead = true;
    rcu_read_lock();
    list_del_rcu(&tunnel->list);

amt_dev_stop():
    list_for_each_entry(tunnel, &amt->tunnel_list, list)
        tunnel->dead = true;

In amt_tunnel_expire(), the store happens before amt_clear_groups() takes
tunnel->lock, so it can run concurrently with this load.

The logic itself still holds, because of the tunnel->lock release/acquire
in amt_clear_groups(). Would KCSAN still report this as a data race?
Should the two writers use WRITE_ONCE()?

> @@ -2576,6 +2717,10 @@ static bool amt_update_handler(struct amt_dev *amt, struct sk_buff *skb)
>  		}
>  
>  		spin_lock_bh(&tunnel->lock);
> +		if (READ_ONCE(tunnel->dead)) {
> +			spin_unlock_bh(&tunnel->lock);
> +			return true;
> +		}
>  		amt_mld_report_handler(amt, skb, tunnel);
>  		spin_unlock_bh(&tunnel->lock);

[Severity: High]
This is a pre-existing issue and was not introduced by this patch. The new
dead checks only cover the Update path. Can the group and source timer
workers still race with group deletion?

amt_group_work() reads its owners and takes the tunnel lock before it has
any lifetime protection:

    struct amt_tunnel_list *tunnel = gnode->tunnel_list;
    struct amt_dev *amt = gnode->amt;
    ...
    spin_lock_bh(&tunnel->lock);

amt_source_work() follows the same pattern.

amt_del_group() does not wait for a worker that is already running:

    if (cancel_delayed_work(&gnode->group_timer))
        dev_put(amt->dev);
    hlist_del_rcu(&gnode->node);
    ...
    kfree_rcu(gnode, rcu);

amt_del_group() is called from several places:

  - amt_clear_groups(), from amt_tunnel_expire() and from amt_dev_stop().
    amt_dev_stop() only sync-cancels tunnel->gc_wq.
  - Leave handlers such as amt_igmpv2_leave_handler().

amt_wq is WQ_UNBOUND.

Two outcomes look possible.

On preemptible kernels, the worker can be preempted before spin_lock_bh().
It is not an RCU reader at that point. After the grace period it can
resume and access a freed gnode or tunnel.

On any kernel, the worker can spin on tunnel->lock while the deleter holds
it. Once the lock is released, the worker sees MCAST_EXCLUDE with empty
source buckets and calls amt_del_group(amt, gnode) a second time.
hlist_del_rcu() then writes through LIST_POISON2, and kfree_rcu() runs
twice on the same gnode.

[ ... ]

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

[ ... ]

> @@ -394,6 +392,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]
Is this comment accurate? What does "the configurable net-next per-source
bound" refer to?

The IFLA_AMT_* attributes in include/uapi/linux/amt.h include no
per-source limit. amt_newlink() applies IFLA_AMT_MAX_TUNNELS only to
amt->max_tunnels. Both amt_tunnel_get_or_create() and amt_request_handler()
compare against the fixed AMT_MAX_TUNNELS_PER_SRC.

The "one tunnel per family" rationale also appears in the
amt_tunnel_get_or_create() comment. It doesn't seem to match the data
model:

  - struct amt_tunnel_list has no family field.
  - Tunnels are keyed only by (ip4, source_port).
  - amt_update_handler() feeds both IGMP and MLD reports into the same
    tunnel.

So a single gateway endpoint uses one tunnel, and the comment doesn't
explain why the value is 16.

Could both comments describe the actual limit and the reason for 16?

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