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

[Eric Dumazet]

On Thu, Oct 1, 2026 at 7:10 PM Omar Ramadan <omar@xxxxxxxxxxxxx> wrote:
>
> amt_send_multicast_data() copies the multicast packet, puts an AMT
> multicast data header and a UDP header in front of it, and sends it with
> udp_tunnel_xmit_skb(). Unlike the other UDP tunnels, it never calls
> udp_tunnel_handle_offloads(), so the copy has neither skb->encapsulation
> nor an SKB_GSO_UDP_TUNNEL* bit set. If the copy is a GSO skb, the lower
> layers see a plain UDP_L4 skb that has an outer UDP header in front of
> it.
>
> A GSO skb only reaches amt_dev_xmit() when tx checksum offload has been
> turned on for the amt device (it is off by default, in which case the
> core segments the packet before ndo_start_xmit), for example with a
> UDP_SEGMENT sender on the relay. The default configuration is not
> affected.
>
> Call udp_tunnel_handle_offloads() on the copy, as bareudp and geneve do.
> The AMT header and the UDP header are pushed after that. Two details
> need care:
>
>  - udp_csum is true, because udp_tunnel_xmit_skb() is called with
>    nocheck set to false. The GSO checksum of the outer UDP header is
>    then completed for every segment, which needs
>    SKB_GSO_UDP_TUNNEL_CSUM.
>
>  - amt is ARPHRD_ETHER (amt_link_setup() ends with ether_setup()), and
>    amt_dev_xmit() pulls the Ethernet header without moving the mac
>    header. skb_copy_expand() keeps the mac header relative to the data,
>    so, going by the code, in the copy it should sit 14 bytes before the
>    inner IP header. The tunnel segmentation derives the length of the
>    outer headers from inner_mac_header - transport_header, which would
>    then be negative. I did not measure either value; what was observed
>    is described below. Reset the mac header on the copy before the
>    inner headers are recorded, so that inner_mac_header is the inner IP
>    header, as it is for the other tunnels that have no link-layer
>    header.
>
> 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. The other UDP tunnels do the same, but the selftest does not
> cover hardware checksumming of such a packet: the egress device in it
> has tx offload off, so skb_checksum_help() completes the checksum in
> software.
>
> This follows the suggestion made by Eric Dumazet on the earlier
> [PATCH net] "amt: do not offer software GSO on the amt device", which
> this replaces.
>
> The problem was found by an LLM-assisted code review of
> drivers/net/amt.c while developing an IPv6 outer transport for amt.
>
> Tested with the selftest in the next patch, in a KVM guest running
> net-next at commit eb0c18404c89 ("amt: pull the AMT header behind the
> transport header in amt_parse_type()"), x86_64, CONFIG_DEBUG_NET=y, AMT
> built in, eleven runs per kernel of the final selftest (22 guest boots,
> two at a time on a busy host). See the next patch for how stable the
> selftest itself has been. The sender is a local UDP_SEGMENT burst
> of eight 1200-byte segments plus a 100-byte tail, 100 bursts, IPv4 and
> IPv6 inner traffic, with "ethtool -K <amt relay dev> tx on" and the
> relay's egress device doing its segmentation in software:
>
>  - Without this patch, every GSO skb (9728 bytes for IPv4, 9748 for
>    IPv6) reached amt_dev_xmit(), none of the 900 datagrams arrived at
>    the listener, the tx_dropped counter of the relay's egress device
>    went up by 100 (one per GSO skb), and nothing was put on the wire.
>    A function-graph trace of one run showed __skb_gso_segment() on that
>    device failing with -EINVAL, from __udp_gso_segment() under
>    udp4_ufo_fragment(), and the skb being freed in validate_xmit_skb().
>
>  - With this patch, all 900 datagrams arrived intact in every run, one
>    AMT message per segment was seen on the wire, none of them larger
>    than the MTU, tx_dropped did not move, and the gateway counted no UDP
>    checksum errors. The trace showed skb_udp_tunnel_segment() doing the
>    outer segmentation.
>
>  - With this patch minus the skb_reset_mac_header() call (two runs, with
>    an earlier version of the selftest), the packets were dropped in the
>    same way as without the patch, and DEBUG_NET warned in
>    skb_udp_tunnel_segment() (pskb_may_pull() with a length above
>    INT_MAX). That fits a negative header length, but the value itself
>    was not printed.
>
>  - With tx offload off (the default), and with non-GSO datagrams with tx
>    on, everything arrived with and without the patch.
>
>  - The existing tools/testing/selftests/net/amt.sh passes with the patch
>    (discovery, IPv4 and IPv6 forwarding, and both torture tests).
>
> Not tested: hardware that offloads UDP tunnel segmentation or the
> checksum of a CHECKSUM_PARTIAL packet, a forwarded UDP GRO packet as the
> GSO source, NETIF_F_GSO_FRAGLIST, KASAN, and the udp_csum=false variant,
> so the choice of true rests on reading the code and on the patched runs
> above being clean, not on a failing false variant. sparse was not run,
> and the existing amt.sh was run only with the patch, not on the
> unpatched kernel. Only the IPv4 outer transport exists in this tree.
>
> Assisted-by: LLM
> Signed-off-by: Omar Ramadan <omar@xxxxxxxxxxxxx>
> ---
>  drivers/net/amt.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> index 0277e4cac..1f0afc11e 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;
> +       }
> +

Okay, but the changelog is far far too long.

Changelogs are for humans (LLM do not care much) and should be shorter.
Also note the use of the 'Suggested-by:' tag.

Something like:

amt_send_multicast_data() encapsulates the multicast packet in
AMT + UDP + IP headers, but unlike other UDP tunnels it never calls
udp_tunnel_handle_offloads().

If tx checksum offload is enabled on the amt device (off by default),
GSO packets (e.g. from a UDP_SEGMENT sender) reach amt_dev_xmit()
unsegmented. They are then sent with neither skb->encapsulation nor
SKB_GSO_UDP_TUNNEL_CSUM set, and the lower device drops them:
__udp_gso_segment() fails because csum_start does not match the
(outer) transport header.

Call udp_tunnel_handle_offloads(skb, true), as other UDP tunnels do.
udp_csum is true because udp_tunnel_xmit_skb() is called with
nocheck == false.

Also reset the mac header of the copy before recording the inner
headers. amt_dev_xmit() pulled the Ethernet header without moving
mac_header, so inner_mac_header would point 14 bytes before the
inner IP header. That makes tnl_hlen negative in
__skb_udp_tunnel_segment().


Suggested-by: Eric Dumazet <edumazet@xxxxxxxxxx>
Assisted-by: LLM
Signed-off-by: Omar Ramadan <omar@xxxxxxxxxxxxx>
---
Testing notes (selftest results, untested configurations, ...) go here.