Re: [PATCH net-next 08/13] amt: send the AMT gateway control plane over IPv6

[netdev-bot+sashiko]

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] Commit-message concern for "amt: send the AMT gateway control
  plane over IPv6": none of the new IPv6 gateway sender code can run at…
- [Low] At this commit the new IPv6 gateway senders use discovery_ipv6
  and remote_ipv6, but the receive side is still IPv4-only.

--

amt: send the AMT gateway control plane over IPv6

This commit adds amt_send_discovery_v6() and amt_send_request_v6(). They
send the gateway Relay Discovery and Request over an IPv6 outer header
through amt_send_ctrl_v6(). It also changes amt_send_membership_update() to
size its headroom for the outer family and send through amt_udp_xmit(), and
adds discovery_ipv6, remote_ipv6 and the remote_ipv6_lock seqlock to struct
amt_dev.

> Send the Discovery and the Request of a gateway with an IPv6 local
> address from amt_send_discovery_v6() and amt_send_request_v6(). Both
> messages are a few bytes with no payload, so they are built on the stack
> and sent with amt_send_ctrl_v6(), the helper that already sends the
> relay's IPv6 Advertisement, rather than with a copy of the IPv4 skb
> construction. The Membership Update carries the gateway's IGMP or MLD
> report, so amt_send_membership_update() keeps building on that skb: it
> sizes the headroom for the outer family and sends through
> amt_udp_xmit(), like the relay's Membership Query and Multicast Data,
> which avoids an IPv6 copy of the function.

[Severity: Low]
This isn't a bug, but could the commit message say that none of the new
IPv6 sender code can run yet?

At this commit nothing writes local_ipv6, so amt_v6() is always false:

drivers/net/amt.c:amt_v6() {
	return IS_ENABLED(CONFIG_IPV6) && !ipv6_addr_any(&amt->local_ipv6);
}

Nothing writes discovery_ipv6 or remote_ipv6 here either. Later patches
fill them in: "amt: receive the AMT gateway control plane over IPv6" and
"amt: add netlink attributes for an IPv6 outer transport".

Earlier patches in the series include a line like "No functional change:
amt_v6() is still false for every device". The first patch also says
"Nothing sets local_ipv6 until the netlink attribute added at the end of
this series". Would a similar line fit here?

The message also leaves out that the IPv4 path of
amt_send_membership_update() now goes through amt_udp_xmit(). As far as I
can tell the behaviour is the same:

- With data=false, amt_udp_xmit() builds the same flowi4 as the removed
  code and calls udp_tunnel_xmit_skb() with the same arguments.
- amt_ip_hlen() still reserves sizeof(struct iphdr).
- amt_dev_xmit() frees the skb on a true return, whether the route lookup
  fails before or after the skb_push().

Could this be described as a refactor with no IPv4 behaviour change?

> diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> index 969ecfe81d7ad..148d1fb369154 100644
> --- a/drivers/net/amt.c
> +++ b/drivers/net/amt.c
> @@ -715,6 +715,50 @@ static int amt_send_ctrl_v6(struct amt_dev *amt, const struct in6_addr *saddr,
>  	return err;
>  }
>  
> +/* The learned IPv6 relay address is written in process context and read
> + * on transmit and receive. A struct in6_addr is not read in one access, so
> + * readers take a snapshot under the seqlock.
> + */

[Severity: Low]
Is the "read on transmit and receive" part accurate at this commit?

The only readers added here are amt_send_request_v6() and the IPv6 branch
of amt_send_membership_update(). In gateway mode, amt_rcv() still checks
the outer source against the IPv4 addresses only:

drivers/net/amt.c:amt_rcv() {
    ...
	if (saddr != amt->discovery_ip) {
    ...
	if (saddr != remote_ip) {
    ...
}

amt_advertisement_handler() also learns only the IPv4 relay address:

	WRITE_ONCE(amt->remote_ip, amta->ip4);

So nothing writes remote_ipv6 yet, and nothing on the receive side reads
it.

The next patch, "amt: receive the AMT gateway control plane over IPv6",
adds amt_from_relay() and amt_set_remote_ipv6(). After that patch the
comment is accurate.

amt_v6() stays false until the netlink patch, which comes after the
receive patch. So this can't be hit in any bisectable state. The comment
just describes code that arrives one commit later. Would it be clearer to
add the receive part of the comment in the next patch?

> +static struct in6_addr amt_get_remote_ipv6(const struct amt_dev *amt)
> +{
> +	struct in6_addr addr;
> +	unsigned int seq;

[ ... ]

> +/* IPv6-outer variant of amt_send_request(); @v6 is the inner family. */
> +static void amt_send_request_v6(struct amt_dev *amt, bool v6)
> +{
> +	const struct in6_addr remote = amt_get_remote_ipv6(amt);

[ ... ]

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