Re: [PATCH net-next 1/2] amt: mark relay data as a UDP tunnel packet before sending it

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 2

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] Fraglist GSO skbs that reach amt_dev_xmit() are still dropped
  after this patch, and each attempt hits a WARN_ON_ONCE.
- [Low] The commit is a bug fix but has no Fixes: tag.
- [Low] The commit message presents a branch change in udp_set_csum() as
  a result of the patch, but that change does not happen.

--

amt: mark relay data as a UDP tunnel packet before sending it

This patch calls udp_tunnel_handle_offloads() on the copy made in
amt_send_multicast_data(). GSO relay traffic then has skb->encapsulation
and SKB_GSO_UDP_TUNNEL_CSUM set before udp_tunnel_xmit_skb() gets it. It
also resets the mac header of the copy so that inner_mac_header points at
the inner IP header, and it replaces an earlier patch that stopped offering
software GSO on the amt device.

> The call also changes what a plain, non-GSO datagram looks like when it
> leaves amt. iptunnel_handle_offloads() sets skb->encapsulation on every
> skb and clears it again only if ip_summed is not CHECKSUM_PARTIAL. A
> CHECKSUM_PARTIAL datagram, which is what the stack hands to the driver
> with tx checksum offload on, now has encapsulation set where it had
> none before, so netif_skb_features() limits the features available for
> it to those in hw_enc_features, and udp_set_csum() takes the local
> checksum offload branch, which leaves the inner checksum to the lower
> device.

[Severity: Low]
Is the udp_set_csum() part of this paragraph accurate? udp_set_csum()
chooses its branch from nocheck, skb_is_gso() and ip_summed only. It never
looks at skb->encapsulation:

net/ipv4/udp.c:udp_set_csum() {
	...
	} else if (skb->ip_summed == CHECKSUM_PARTIAL) {
		uh->check = 0;
		uh->check = udp_v4_check(len, saddr, daddr, lco_csum(skb));
	...
}

lco_csum() doesn't read encapsulation either. amt passes nocheck=false to
udp_tunnel_xmit_skb(), so a non-GSO CHECKSUM_PARTIAL datagram already went
through the lco_csum() branch before this patch. The inner checksum was
already left to the lower device.

The real change seems to be that skb->encapsulation is now set and
inner_mac_header has moved. As a result, the lower device's
hw_enc_features now decide whether that checksum is offloaded.

Could the message say only that?

> Assisted-by: LLM
> Signed-off-by: Omar Ramadan <omar@xxxxxxxxxxxxx>

[Severity: Low]
Should this have a Fixes: tag? The relay data path that never calls
udp_tunnel_handle_offloads() came in with:

Fixes: cbc21dc1cfe9 ("amt: add data plane of amt interface")

The commit message says every GSO datagram is dropped on an unpatched
kernel once tx checksum offload is on for the amt device. amt_link_setup()
allows that because it puts NETIF_F_HW_CSUM in hw_features.

The previous amt fix in this tree, eb0c18404c89, has the same Fixes: tag.
This patch also replaces an earlier [PATCH net] submission. Without the
tag, stable backporters have nothing that tells them which kernels are
affected.

> diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> index 0277e4cac39bd..1f0afc11ea0c1 100644
> --- a/drivers/net/amt.c
> +++ b/drivers/net/amt.c
> @@ -1078,7 +1078,18 @@ static void amt_send_multicast_data(struct amt_dev *amt,
>  	if (!skb)
>  		return;
>  
> +	/* amt_dev_xmit() pulled the Ethernet header without moving the mac
> +	 * header, so the copy's mac header sits 14 bytes before the inner IP
> +	 * header. Make it coincide with it, as the inner segmentation code
> +	 * expects for a device without a link-layer header.
> +	 */
> +	skb_reset_mac_header(skb);
>  	skb_reset_inner_headers(skb);
> +	if (udp_tunnel_handle_offloads(skb, true)) {
> +		kfree_skb(skb);
> +		return;
> +	}

[Severity: Medium]
Are fraglist GSO skbs still dropped after this patch? They never reach
this new call.

amt_link_setup() advertises NETIF_F_GSO_SOFTWARE, and that mask includes
NETIF_F_GSO_FRAGLIST. It also lets the user turn on NETIF_F_HW_CSUM and
NETIF_F_FRAGLIST:

	dev->features		|= NETIF_F_GSO_SOFTWARE;
	dev->hw_features	|= NETIF_F_SG | NETIF_F_HW_CSUM;
	dev->hw_features	|= NETIF_F_FRAGLIST | NETIF_F_RXCSUM;

When rx-gro-list is on for the ingress device, udp_gro_complete() builds
forwarded SKB_GSO_FRAGLIST | SKB_GSO_UDP_L4 skbs with
CHECKSUM_UNNECESSARY. Suppose multicast routing forwards one of them to an
amt relay that has tx checksum and fraglist turned on. skb_gso_ok()
passes, so the skb arrives at amt_dev_xmit() unsegmented.

The copy made just above this hunk then fails:

amt_dev_xmit()
  amt_send_multicast_data()
    skb_copy_expand()
      if (WARN_ON_ONCE(skb_shinfo(skb)->gso_type & SKB_GSO_FRAGLIST))
          return NULL;

amt_send_multicast_data() returns before udp_tunnel_handle_offloads(), so
nothing is relayed and the WARN_ON_ONCE fires.

The earlier approach ("amt: do not offer software GSO on the amt device")
would have handled this case. The commit message lists
NETIF_F_GSO_FRAGLIST as untested, and the selftest patch that follows does
not touch drivers/net.

Would it close the gap to drop NETIF_F_GSO_FRAGLIST from the advertised
features (for example NETIF_F_GSO_SOFTWARE & ~NETIF_F_GSO_FRAGLIST)? The
core would then segment these skbs before ndo_start_xmit.

[ ... ]

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