On Thu, Oct 1, 2026 at 7:10 PM Omar Ramadan <[email protected]> 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 <[email protected]> > --- > 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 <[email protected]> Assisted-by: LLM Signed-off-by: Omar Ramadan <[email protected]> --- Testing notes (selftest results, untested configurations, ...) go here.
