Re: [PATCH net v3 2/4] ip_tunnel: move the TCP fixed-ID flag to the inner header

Re: [PATCH net v3 2/4] ip_tunnel: move the TCP fixed-ID flag to the inner header

[netdev-bot+sashiko]

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

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

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).

- [High] The new hunk in iptunnel_handle_offloads() assumes that whenever
  skb->encapsulation is 0, SKB_GSO_TCP_FIXEDID describes the skb's…
- [Medium] ip_tunnel: iptunnel_handle_offloads() breaks inner fixed ID
  for GRE/UDP GSO

--

ip_tunnel: move the TCP fixed-ID flag to the inner header

When a GSO packet is encapsulated, iptunnel_handle_offloads() now moves
SKB_GSO_TCP_FIXEDID to SKB_GSO_TCP_FIXEDID_INNER. The aim is that software
segmentation keeps fixed IDs on the header that GRO merged with fixed IDs,
which becomes the inner header after encapsulation.

> diff --git a/net/ipv4/ip_tunnel_core.c b/net/ipv4/ip_tunnel_core.c
> index bab42b9e277f7..752ddba1298b6 100644
> --- a/net/ipv4/ip_tunnel_core.c
> +++ b/net/ipv4/ip_tunnel_core.c
> @@ -174,18 +174,29 @@ EXPORT_SYMBOL_GPL(iptunnel_metadata_reply);
>  int iptunnel_handle_offloads(struct sk_buff *skb,
>  			     int gso_type_mask)
>  {
> +	bool inner = false;
>  	int err;
>
>  	if (likely(!skb->encapsulation)) {
>  		skb_reset_inner_headers(skb);
>  		skb->encapsulation = 1;
> +		inner = true;
>  	}
>
>  	if (skb_is_gso(skb)) {
> +		struct skb_shared_info *shinfo;
> +
>  		err = skb_header_unclone(skb, GFP_ATOMIC);
>  		if (unlikely(err))
>  			return err;
> -		skb_shinfo(skb)->gso_type |= gso_type_mask;
> +
> +		shinfo = skb_shinfo(skb);
> +		/* The header SKB_GSO_TCP_FIXEDID described is now the inner one */
> +		if (inner && (shinfo->gso_type & SKB_GSO_TCP_FIXEDID)) {
> +			shinfo->gso_type &= ~SKB_GSO_TCP_FIXEDID;
> +			shinfo->gso_type |= SKB_GSO_TCP_FIXEDID_INNER;
> +		}

[Severity: Medium]
iptunnel_handle_offloads() is not only used by the IP-in-IP tunnels.
gre_handle_offloads() in ip_gre and ip6_gre, vxlan_build_skb(),
__fou_build_header(), __gue_build_header() and
udp_tunnel_handle_offloads() (geneve, bareudp) all call it. That means
the conversion to SKB_GSO_TCP_FIXEDID_INNER now also happens for GRE
and UDP tunnels. Does their software GSO path ever look at the inner
bit?

inet_gso_segment() picks the bit from the nesting level:

	encap = SKB_GSO_CB(skb)->encap_level > 0;
	...
	fixedid = !!(skb_shinfo(skb)->gso_type &
		     (SKB_GSO_TCP_FIXEDID << encap));

ipip_gso_segment() and ip4ip6_gso_segment() keep encap_level non-zero
when they recurse into inet_gso_segment() for the inner IPv4 header.
gre_gso_segment() and __skb_udp_tunnel_segment() do not. Both set up
the inner skb with:

	skb->encapsulation = 0;
	SKB_GSO_CB(skb)->encap_level = 0;

They do this before calling skb_mac_gso_segment() or
gso_inner_segment(). So when inet_gso_segment() runs on the inner IPv4
header of a GRE or UDP tunnel packet, encap is 0. It then tests
SKB_GSO_TCP_FIXEDID, which this patch has just cleared.
SKB_GSO_TCP_FIXEDID_INNER is never consulted for that header.

Take a TCPv4 flow that GRO merged with fixed IDs (tcp4_gro_complete()
sets SKB_GSO_TCP_FIXEDID). Forward it into a GRE or VXLAN tunnel device
where it is segmented in software:

  ipgre_xmit() / vxlan_xmit_one() -> iptunnel_handle_offloads()
    (SKB_GSO_TCP_FIXEDID becomes SKB_GSO_TCP_FIXEDID_INNER)
  __skb_gso_segment() -> inet_gso_segment() for the outer header
    -> gre_gso_segment() / __skb_udp_tunnel_segment()
       (encap_level reset to 0)
    -> inet_gso_segment() for the inner header
       (encap == 0, so fixedid == 0)

Wouldn't the inner header then get iph->id = htons(id++) on every
segment? That is the inner ID rewriting this patch sets out to avoid.

For an IPv6 outer header (ip6_gre, VXLAN6, GENEVE6, bareudp over IPv6,
FOU6/GUE6) carrying inner IPv4, this looks like a plain regression.
Before the patch SKB_GSO_TCP_FIXEDID stayed set, the inner
inet_gso_segment() ran with encap_level 0, and the fixed inner IDs
were preserved. With the patch they start incrementing.

Should the move be limited to the tunnels whose GSO path keeps
encap_level non-zero for the inner header (the SKB_GSO_IPXIP4 and
SKB_GSO_IPXIP6 cases)? Or is the intent to also change
gre_gso_segment() and __skb_udp_tunnel_segment() so that the inner
inet_gso_segment() tests SKB_GSO_TCP_FIXEDID_INNER? Either way, the
bit written here and the bit read at segmentation time need to agree
for every caller of iptunnel_handle_offloads().

[Severity: High]
This assumes SKB_GSO_TCP_FIXEDID always describes the current outermost
header when skb->encapsulation is 0. Does the decap side keep that true?

iptunnel_pull_offloads() in include/net/ip_tunnels.h only clears the
encap GSO bits and resets encapsulation:

include/net/ip_tunnels.h:iptunnel_pull_offloads() {
    ...
		skb_shinfo(skb)->gso_type &= ~(NETIF_F_GSO_ENCAP_ALL >>
					       NETIF_F_GSO_SHIFT);
	}

	skb->encapsulation = 0;
    ...
}

It does not move SKB_GSO_TCP_FIXEDID_INNER back to SKB_GSO_TCP_FIXEDID.
It also does not drop the SKB_GSO_TCP_FIXEDID bit that belonged to the
outer header it removed. This function is reached from
__iptunnel_pull_header() (VXLAN, GRE, Geneve, IPIP), fou_recv_pull(),
gue_udp_recv() and the seg6 decap path. bpf_skb_net_shrink() in
net/core/filter.c behaves the same way.

GRO sets the outer bit independently in tcp4_gro_complete():

	skb_shinfo(skb)->gso_type |= SKB_GSO_TCPV4 |
			(NAPI_GRO_CB(skb)->ip_fixedid * SKB_GSO_TCP_FIXEDID);

inet_gso_segment() then picks a bit based on the nesting level:

	fixedid = !!(skb_shinfo(skb)->gso_type & (SKB_GSO_TCP_FIXEDID << encap));

Take a decap and re-encap gateway (for example VXLAN to GRE, or IPIP to
IPIP) where the received outer header had fixed IDs and the inner header
had incrementing IDs:

  vxlan/gre rcv -> __iptunnel_pull_header() -> iptunnel_pull_offloads()
    (the stale outer SKB_GSO_TCP_FIXEDID survives, encapsulation = 0)
  gro_cells_receive() -> ... -> ip_forward()
  ipgre_xmit() -> gre_handle_offloads() -> iptunnel_handle_offloads()
    (inner == true, so the stale bit becomes SKB_GSO_TCP_FIXEDID_INNER)
  inet_gso_segment() with encap == 1
    (fixedid is set for the inner header)

Wouldn't this give every segment the same inner IPv4 ID, even though
those IDs originally incremented? Before this patch, the wrong state
ended up on the new outer header. With the patch it ends up on the inner
one.

There is a second case. Take an skb that is encapsulated and then
decapsulated again without being segmented, such as a tunnel into a
netns over veth. Before this patch it kept SKB_GSO_TCP_FIXEDID. It now
carries SKB_GSO_TCP_FIXEDID_INNER, and that bit is ignored after decap.
Won't IDs that were originally fixed now come out incrementing?

A related problem predates this patch and has been there since
3271f19bf7b9: a decapsulated, forwarded GSO packet is segmented using
the fixed-ID state of the removed outer header, and its own
SKB_GSO_TCP_FIXEDID_INNER state is ignored.

Would a matching decap step in iptunnel_pull_offloads() and
bpf_skb_net_shrink() fix this? That step would set SKB_GSO_TCP_FIXEDID
from SKB_GSO_TCP_FIXEDID_INNER and clear the inner bit. Without it, the
fixed-ID flags can still describe the wrong header after crossing a
tunnel boundary.

> +		shinfo->gso_type |= gso_type_mask;
>  		return 0;
>  	}
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1792293419.git.julius%40bairaktaris.de

[PATCH net v3 0/4] ipmr: forward GSO multicast packets

[Julius Bairaktaris]

With fraglist GRO or UDP GRO forwarding on, a routed IPv4 multicast
stream arrives as GSO packets. ipmr_prepare_xmit() drops every DF
aggregate larger than the MTU, and ip_encap() sets no tunnel GSO state,
so a GSO packet sent to a VIFF_TUNNEL vif is dropped during
segmentation. With that state set, a fraglist aggregate reaches
skb_segment_list() with tunnel headers, which can write before the head
of a frag_list member, and a TCP GRO packet keeps its fixed-ID flag on
what is now the outer header. Both also happen today when such a packet
is routed into a tunnel device.

Patch 1 makes room for the tunnel headers in skb_segment_list().
Patch 2 moves the TCP fixed-ID flag to the inner header on encapsulation.
Patch 3 sets up IPXIP4 GSO state and outer IP IDs in ip_encap().
Patch 4 lets DF GSO packets pass when their segments fit the MTU.

Tested in QEMU on veth, n=3 per arm: an MFC entry to a plain vif and a
tunnel vif, 4000 1316-byte datagrams per run, fraglist GRO, UDP GRO
forwarding and UDP_SEGMENT, DF set and clear, software segmentation on
egress; a fraglist stream routed into VXLAN over IPv4 and IPv6; and a
TCP stream with fixed IP IDs, merged by GRO and routed into an ipip
device with DF clear.
- net (7ec48f396a1d): DF aggregates dropped on both vifs (FragFails),
  non-DF aggregates dropped on the tunnel vif (tx_dropped); VXLAN:
  KASAN slab-out-of-bounds write in skb_segment_list() in 6 of 6
  runs, then a panic.
- v2: outer IP IDs repeat on the tunnel vif in 18 of 18 runs.
- without patch 2: TCP over ipip repeats outer IP IDs (831 unique of
  14666) and increments the fixed inner IDs, 3 of 3 runs.
- this series: every segment sent on both vifs, FragFails 0, no tx
  drops, outer IP IDs unique in 18 of 18 runs; VXLAN: no KASAN report
  in 6 of 6 runs; TCP over ipip: outer IDs unique, inner IDs kept, 3 of
  3 runs.

Changes in v3:
- Patch 1: name the VXLAN out-of-bounds write (Sashiko).
- Add patch 2 (Sashiko).
- Patch 3: reserve one outer IP ID per segment (Sashiko).
- v2: https://lore.kernel.org/netdev/20261011162957.1328474-1-julius@bairaktaris.de/

Changes in v2:
- Add patches 1 and 3; v1 let DF GSO packets reach ip_encap() on tunnel
  vifs, where they were dropped during segmentation (Sashiko).
- Keep the additive MTU comparison and check mtu > encap before
  subtracting, so a route MTU below 20 cannot wrap (Sashiko).
- v1: https://lore.kernel.org/netdev/20261010131842.2157365-1-julius@bairaktaris.de/

Julius Bairaktaris (4):
  net: make room for the tunnel header in skb_segment_list()
  ip_tunnel: move the TCP fixed-ID flag to the inner header
  ipmr: set up GSO state for tunnel vifs
  ipmr: do not drop GSO packets that fit the MTU after segmentation

 net/core/skbuff.c         |  4 ++++
 net/ipv4/ip_tunnel_core.c | 13 ++++++++++++-
 net/ipv4/ipmr.c           | 23 +++++++++++++++++------
 3 files changed, 33 insertions(+), 7 deletions(-)


base-commit: 7ec48f396a1d89532c0537bc38b408b8c31ee37f
-- 
2.53.0

[PATCH net v3 2/4] ip_tunnel: move the TCP fixed-ID flag to the inner header

[Julius Bairaktaris]

SKB_GSO_TCP_FIXEDID describes the outer header of an encapsulated GSO
packet and SKB_GSO_TCP_FIXEDID_INNER the inner one. TCP GRO sets
SKB_GSO_TCP_FIXEDID for the header it merged. When such a packet is
forwarded into a tunnel, iptunnel_handle_offloads() makes that header the
inner one but leaves the flag, so software segmentation gives every
segment the outer ID of the first and increments the inner IDs.

Move the flag to SKB_GSO_TCP_FIXEDID_INNER when the packet becomes
encapsulated.

Fixes: 3271f19bf7b9 ("net: gso: restore ids of outer ip headers correctly")
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Julius Bairaktaris <julius@bairaktaris.de>
---
 net/ipv4/ip_tunnel_core.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/ip_tunnel_core.c b/net/ipv4/ip_tunnel_core.c
index bab42b9e277f7..752ddba1298b6 100644
--- a/net/ipv4/ip_tunnel_core.c
+++ b/net/ipv4/ip_tunnel_core.c
@@ -174,18 +174,29 @@ EXPORT_SYMBOL_GPL(iptunnel_metadata_reply);
 int iptunnel_handle_offloads(struct sk_buff *skb,
 			     int gso_type_mask)
 {
+	bool inner = false;
 	int err;
 
 	if (likely(!skb->encapsulation)) {
 		skb_reset_inner_headers(skb);
 		skb->encapsulation = 1;
+		inner = true;
 	}
 
 	if (skb_is_gso(skb)) {
+		struct skb_shared_info *shinfo;
+
 		err = skb_header_unclone(skb, GFP_ATOMIC);
 		if (unlikely(err))
 			return err;
-		skb_shinfo(skb)->gso_type |= gso_type_mask;
+
+		shinfo = skb_shinfo(skb);
+		/* The header SKB_GSO_TCP_FIXEDID described is now the inner one */
+		if (inner && (shinfo->gso_type & SKB_GSO_TCP_FIXEDID)) {
+			shinfo->gso_type &= ~SKB_GSO_TCP_FIXEDID;
+			shinfo->gso_type |= SKB_GSO_TCP_FIXEDID_INNER;
+		}
+		shinfo->gso_type |= gso_type_mask;
 		return 0;
 	}
 
-- 
2.53.0