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

[Omar Ramadan <omar@blockcast.net>]

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@blockcast.net>
---
 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;
+	}
+
 	memset(&fl4, 0, sizeof(struct flowi4));
 	fl4.flowi4_oif         = amt->stream_dev->ifindex;
 	fl4.daddr              = tunnel->ip4;
-- 
2.43.0