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

[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] The commit fixes a user-visible bug but has no Fixes: trailer.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but fraglist GSO
  skbs that reach the AMT relay unsegmented, the same situation the…

--

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

This patch makes amt_send_multicast_data() call
udp_tunnel_handle_offloads(skb, true), so GSO packets relayed with tx
checksum offload enabled are marked as UDP tunnel packets. It also resets
the mac header of the copied skb before recording the inner headers, so
inner_mac_header lines up with the inner IP header.

> Suggested-by: Eric Dumazet <edumazet@kernel.org>
> Assisted-by: LLM
> Signed-off-by: Omar Ramadan <omar@blockcast.net>

[Severity: Low]
Should this have a Fixes: tag?

The send path being fixed was added by commit cbc21dc1cfe9 ("amt: add data
plane of amt interface"). That commit introduced amt_send_multicast_data()
with skb_copy_expand(), skb_reset_inner_headers() and
udp_tunnel_xmit_skb(), but no udp_tunnel_handle_offloads().

Suggested trailer:

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

The notes say this was tested on net-next. Since it fixes packet loss
that users can see, should it target net instead?

> diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> index 0277e4cac39bd..a8236d40c2e70 100644
> --- a/drivers/net/amt.c
> +++ b/drivers/net/amt.c
> @@ -1078,7 +1078,17 @@ static void amt_send_multicast_data(struct amt_dev *amt,
>  	if (!skb)
>  		return;
>  

[Severity: High]
This is a pre-existing issue, but it is closely related to this patch.
Fraglist GSO skbs can reach the relay unsegmented in the same way, yet they
never get to the new udp_tunnel_handle_offloads() call. The
skb_copy_expand() just above rejects them first:

net/core/skbuff.c:skb_copy_expand() {
    ...
	if (WARN_ON_ONCE(skb_shinfo(skb)->gso_type & SKB_GSO_FRAGLIST))
		return NULL;
    ...
}

amt_link_setup() advertises fraglist GSO:

	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;
	dev->hw_features	|= NETIF_F_GSO_SOFTWARE;

NETIF_F_GSO_SOFTWARE includes NETIF_F_GSO_FRAGLIST and
NETIF_F_GSO_UDP_L4.

Suppose the relay's ingress device has rx-gro-list enabled. For
forwarded multicast there is no local socket, so udp_gro_receive() sets
is_flist. The result is an skb with gso_type
SKB_GSO_UDP_L4 | SKB_GSO_FRAGLIST and a frag_list.

ipmr then forwards it to the amt vif. ipmr_prepare_xmit() only drops
oversized skbs when DF is set, and the sender controls DF.

Now suppose tx-checksumming and tx-scatter-gather-fraglist are enabled on
amt. skb_gso_ok() then returns true, so validate_xmit_skb() leaves the skb
unsegmented, and the path becomes:

amt_dev_xmit()
  amt_send_multicast_data()
    skb_copy_expand()
      WARN_ON_ONCE(... SKB_GSO_FRAGLIST)
      return NULL

The function then returns without any drop accounting. Once those
non-default offloads are on, can ordinary remote multicast traffic trigger
this WARN? With panic_on_warn, that would panic the relay.

The notes list "a forwarded GRO packet as the GSO source" as untested.
Would it make sense to also stop advertising NETIF_F_GSO_FRAGLIST and
NETIF_F_FRAGLIST on amt? Another option is to segment or linearize
fraglist skbs before the copy.

> +	/* amt_dev_xmit() pulled the Ethernet header without moving the mac
> +	 * header. The tunnelled payload has no link-layer header, so the
> +	 * inner mac header must coincide with the inner IP header.
> +	 */
> +	skb_reset_mac_header(skb);
>  	skb_reset_inner_headers(skb);
> +	if (udp_tunnel_handle_offloads(skb, true)) {
> +		kfree_skb(skb);
> +		return;
> +	}
> +

[ ... ]

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