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