Re: [PATCH net v3 4/4] ipmr: do not drop GSO packets that fit the MTU after segmentation

Re: [PATCH net v3 4/4] ipmr: do not drop GSO packets that fit the MTU after segmentation

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · 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).

- [Medium] The new GSO exemption in ipmr_prepare_xmit()
  (net/ipv4/ipmr.c:1952-1953) trusts skb_gso_validate_network_len() to…

--

ipmr: do not drop GSO packets that fit the MTU after segmentation

This patch changes ipmr_prepare_xmit() so that DF-marked GSO packets
longer than the route MTU are no longer dropped when
skb_gso_validate_network_len() says the segments will fit. This matches
what ip_forward() already does. On tunnel vifs it leaves room for the
outer header.

> diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
> index 6f2eb571f30f7..0a2c573ceeb79 100644
> --- a/net/ipv4/ipmr.c
> +++ b/net/ipv4/ipmr.c

[ ... ]

> @@ -1946,7 +1947,10 @@ static int ipmr_prepare_xmit(struct net *net, struct mr_table *mrt,
>  			return -1;
>  	}
>  
> -	if (skb->len+encap > dst4_mtu(&rt->dst) && (ntohs(iph->frag_off) & IP_DF)) {
> +	mtu = dst4_mtu(&rt->dst);
> +	if (skb->len + encap > mtu && (ntohs(iph->frag_off) & IP_DF) &&
> +	    !(skb_is_gso(skb) && mtu > encap &&
> +	      skb_gso_validate_network_len(skb, mtu - encap))) {

[Severity: Medium]
Can this exemption let oversized DF segments out of ipmr for encapsulated
UDP-in-UDP aggregates?

skb_gso_validate_network_len() gets the segment length from
skb_gso_transport_seglen(). The encapsulated branch of that helper only
adds an inner L4 header length for TCP:

net/core/gso.c:skb_gso_transport_seglen() {
    ...
	if (skb->encapsulation) {
		thlen = skb_inner_transport_header(skb) -
			skb_transport_header(skb);

		if (likely(shinfo->gso_type & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6)))
			thlen += inner_tcp_hdrlen(skb);
	} else if (likely(shinfo->gso_type & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6))) {
		thlen = tcp_hdrlen(skb);
	} else if (unlikely(skb_is_gso_sctp(skb))) {
		thlen = sizeof(struct sctphdr);
	} else if (shinfo->gso_type & SKB_GSO_UDP_L4) {
		thlen = sizeof(struct udphdr);
	}
    ...
}

If the inner aggregate is SKB_GSO_UDP_L4, gso_size does not include the
8-byte inner UDP header. Nothing adds it back, so each segment is
counted 8 bytes too short.

Receive-side GRO builds aggregates like this when a local tunnel socket
exists, for example a VXLAN device bound to the port.
udp_gro_complete() sets skb->encapsulation = 1. The inner
udp_gro_complete_segment() then sets inner_transport_header and
SKB_GSO_UDP_L4.

Here is an example: outer IPv4 (20) + UDP (8) + VXLAN (8) + inner
Ethernet (14) + inner IPv4 (20) + inner UDP (8) + 1424 bytes of payload.
Each real segment is 1502 bytes, but the helper reports 1494.

Say the multicast route sends such a DF aggregate to a non-tunnel vif
with MTU 1500, and the ingress MTU is at least 1502. The old check
dropped it. The new check lets it through.

Later, ip_finish_output_gso() calls the same helper:

net/ipv4/ip_output.c:ip_finish_output_gso() {
    ...
	if (skb_gso_validate_network_len(skb, mtu))
		return ip_finish_output2(net, sk, skb);
    ...
}

So it takes the fast path, and 1502-byte DF segments go out on a
1500-MTU link.

The undercount in skb_gso_transport_seglen() is older than this patch.
It also affects ip_exceeds_mtu() on the unicast forwarding path. This
patch is the first to make the multicast MTU decision depend on the
helper.

Should the inner UDP header length be accounted for in the
skb->encapsulation branch of skb_gso_transport_seglen()?

>  		/* Do not fragment multicasts. Alas, IPv4 does not
>  		 * allow to send ICMP, so that packets will disappear
>  		 * to blackhole.

-- 
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 4/4] ipmr: do not drop GSO packets that fit the MTU after segmentation

[Julius Bairaktaris]

ipmr_prepare_xmit() drops a packet with DF set when skb->len exceeds the
route MTU, without checking whether it is a GSO packet. A UDP multicast
stream received with fraglist GRO or UDP GRO forwarding enabled is
aggregated into GSO packets, and every aggregate is dropped with
FragFails, while ip_forward() lets the same packets pass by checking
skb_gso_validate_network_len(). Do the same for multicast routing,
leaving room for the outer header on tunnel vifs. ip6mr has no such
check and leaves GSO packets to ip6_output().

On an IPQ8074 router with fraglist GRO on, a 10 Mbit/s stream of
1316-byte DF datagrams routed by igmpproxy loses 14.5 to 15.5% (n=3)
and 0% with this change.

Fixes: 9fd1ff5d2ac7 ("udp: Support UDP fraglist GRO/GSO.")
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Julius Bairaktaris <julius@bairaktaris.de>
---
 net/ipv4/ipmr.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index 6f2eb571f30f7..0a2c573ceeb79 100644
--- a/net/ipv4/ipmr.c
+++ b/net/ipv4/ipmr.c
@@ -1914,6 +1914,7 @@ static int ipmr_prepare_xmit(struct net *net, struct mr_table *mrt,
 	struct rtable *rt;
 	struct flowi4 fl4;
 	int    encap = 0;
+	unsigned int mtu;
 
 	vif_dev = vif_dev_read(vif);
 	if (!vif_dev)
@@ -1946,7 +1947,10 @@ static int ipmr_prepare_xmit(struct net *net, struct mr_table *mrt,
 			return -1;
 	}
 
-	if (skb->len+encap > dst4_mtu(&rt->dst) && (ntohs(iph->frag_off) & IP_DF)) {
+	mtu = dst4_mtu(&rt->dst);
+	if (skb->len + encap > mtu && (ntohs(iph->frag_off) & IP_DF) &&
+	    !(skb_is_gso(skb) && mtu > encap &&
+	      skb_gso_validate_network_len(skb, mtu - encap))) {
 		/* Do not fragment multicasts. Alas, IPv4 does not
 		 * allow to send ICMP, so that packets will disappear
 		 * to blackhole.
-- 
2.53.0