Re: [PATCH net v7 6/6] amt: create relay tunnels only from authenticated Updates

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] In drivers/net/amt.c, amt_request_handler() now drops a Request
  without any reply when the relay cannot admit a new endpoint.
- [Low] The comment above test_query_egress() in
  tools/testing/selftests/net/amt.sh (lines 256-259) still says: "The…

--

amt: create relay tunnels only from authenticated Updates

The relay no longer creates tunnel state from unauthenticated Requests.
Instead, the Query MAC is derived from the source address, port, nonce and
one of four rotating secret generations. Tunnels are created or refreshed
only after the Membership Update's MAC and encapsulated IGMP/MLD message
are verified, with global and per-source limits.

> diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> index de5b7dbe3a20d..8014798b33167 100644
> --- a/drivers/net/amt.c
> +++ b/drivers/net/amt.c

[ ... ]

> @@ -2888,59 +3059,45 @@ static bool amt_request_handler(struct amt_dev *amt, struct sk_buff *skb)
>  	if (amtrh->reserved1 || amtrh->reserved2 || amtrh->version)
>  		return true;
>  
> -	list_for_each_entry_rcu(tunnel, &amt->tunnel_list, list)
> -		if (tunnel->ip4 == iph->saddr)
> -			goto send;
> -
> -	spin_lock_bh(&amt->lock);
> -	if (amt->nr_tunnels >= amt->max_tunnels) {
> -		spin_unlock_bh(&amt->lock);
> -		icmp_ndo_send(skb, ICMP_DEST_UNREACH, ICMP_HOST_UNREACH, 0);
> -		return true;
> -	}

[ ... ]

> +	rcu_read_lock();
> +	list_for_each_entry_rcu(tunnel, &amt->tunnel_list, list) {
> +		if (tunnel->ip4 != iph->saddr || READ_ONCE(tunnel->dead))
> +			continue;
> +		per_src++;
> +		if (tunnel->source_port == udph->source)
> +			have = true;
> +	}
> +	rcu_read_unlock();
>  
> -	if (!netif_running(amt->dev) || !netif_running(amt->stream_dev))
> +	/* Withhold the Query when a new endpoint cannot get a tunnel. */
> +	if (!have && (full || per_src >= amt_max_tunnels_per_src(amt)))
>  		return true;
>  

[Severity: Low]
amt_request_handler() now drops the Request with no reply in two cases:
the table is full, or the source address already holds
amt_max_tunnels_per_src() live tunnels. Before this patch, a full table
got an ICMP Host Unreachable through icmp_ndo_send(). The new per-source
cap case is also silent, and gateways that share a NAT address can hit it.

The commit message only compares silence with ICMP. RFC 7450 also defines
an in-band signal for this situation: the Limit (L) flag in the Membership
Query. amt_send_membership_query() still hard-codes it to zero:

	amtmq->l	= 0;

Could the relay answer with an L=1 Query here instead of dropping the
Request? The relay already sends full Queries to unauthenticated sources
on its normal path. So the reason given for staying silent ("a source
address nothing has authenticated yet") doesn't seem to rule out an L=1
Query.

With no signal at all, a gateway that is refused can't tell a capacity
refusal from packet loss. It keeps retrying Requests with backoff instead
of moving to another relay.

The in-tree gateway's timing changes too. amt_err_lookup() used to react
to the ICMP for a Request by retrying immediately:

	case AMT_MSG_REQUEST:
	case AMT_MSG_MEMBERSHIP_UPDATE:
		if (READ_ONCE(amt->status) >= AMT_STATUS_RECEIVED_ADVERTISEMENT)
			mod_delayed_work(amt_wq, &amt->req_wq, 0);

That reached the AMT_MAX_REQ_COUNT reset to discovery quickly. Now the
gateway waits out the exponential backoff in amt_event_send_request()
before it falls back to discovery.

The in-tree amt_membership_query_handler() ignores amtmq->l. Setting L
would therefore only help third-party gateways unless the gateway side is
also updated.

> +	mac = siphash_3u32((__force u32)iph->saddr,
> +			   (__force u32)udph->source,
> +			   (__force u32)amtrh->nonce,
> +			   &key) >> 16;
>  	if (!amtrh->p)
> -		amt_send_igmp_gq(amt, tunnel);
> +		amt_send_igmp_gq(amt, iph->saddr, udph->source, amtrh->nonce,
> +				 mac);
>  	else
> -		amt_send_mld_gq(amt, tunnel);
> +		amt_send_mld_gq(amt, iph->saddr, udph->source, amtrh->nonce,
> +				mac);

[Severity: Low]
This isn't a bug, but the comment above test_query_egress() in
tools/testing/selftests/net/amt.sh is now out of date:

# 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() neither looks up nor creates a
tunnel for the Query. The daddr, dport, nonce and mac values are passed
by value, and struct amt_tunnel_list no longer has nonce, key or mac
fields. The check the test makes still holds, but should the comment be
updated to match?

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