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

[netdev-bot+sashiko]

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

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

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] On VIFF_TUNNEL vifs the patch does not stop the drop it claims
  to fix.
- [Low] The patch rewrites the DF/MTU check in ipmr_prepare_xmit() from
  `skb->len + encap > dst4_mtu(&rt->dst)` (which cannot wrap) to `mtu =…

--

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

This patch changes ipmr_prepare_xmit() so a DF packet larger than the
route MTU is no longer dropped if it is a GSO packet whose segments fit
that MTU. This matches the skb_gso_validate_network_len() check in
ip_forward(). The MTU used for the check is now dst4_mtu() minus the
tunnel encapsulation overhead.

> diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
> index b9c544d48c452..06c37b4e653bf 100644
> --- a/net/ipv4/ipmr.c
> +++ b/net/ipv4/ipmr.c
> @@ -1908,6 +1908,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)
> @@ -1940,7 +1941,9 @@ 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) - encap;
> +	if (skb->len > mtu && (ntohs(iph->frag_off) & IP_DF) &&
> +	    !(skb_is_gso(skb) && skb_gso_validate_network_len(skb, mtu))) {

[Severity: High]
The new mtu value subtracts encap, so this check seems meant to cover
VIFF_TUNNEL vifs too. Can a DF GSO aggregate that passes it actually be
transmitted on a tunnel vif?

Further down in ipmr_prepare_xmit(), such an skb goes to ip_encap():

	if (vif->flags & VIFF_TUNNEL) {
		ip_encap(net, skb, vif->local, vif->remote);

ip_encap() only adds the outer header:

	skb_push(skb, sizeof(struct iphdr));
	skb->transport_header = skb->network_header;
	skb_reset_network_header(skb);
	...
	iph->protocol	=	IPPROTO_IPIP;
	...
	iph->tot_len	=	htons(skb->len);

It does not add SKB_GSO_IPXIP4 to gso_type, set skb->encapsulation or
set the inner header offsets. ipip_tunnel_xmit() does set up that state,
by calling iptunnel_handle_offloads(skb, SKB_GSO_IPXIP4) first.

Software segmentation runs for fraglist GSO, and on any device without
hardware USO. In that case the path is:

validate_xmit_skb()
  skb_gso_segment()
    inet_gso_segment()
      ipip_gso_segment()

and ipip_gso_segment() rejects the skb:

	if (!(skb_shinfo(skb)->gso_type & SKB_GSO_IPXIP4))
		return ERR_PTR(-EINVAL);

validate_xmit_skb() then frees the skb and only increments tx_dropped.
The skb_gso_network_seglen() check in ip_finish_output_gso() does not
catch this either. After ip_encap(), transport minus network covers only
the 20-byte outer header.

So on tunnel vifs, are these packets still dropped, just no longer
counted in FragFails? Meanwhile vif->pkt_out, vif->bytes_out,
tx_packets and OUTFORWDATAGRAMS count them as sent.

There is also a hardware case, which I have not fully traced. If the
egress device advertises NETIF_F_GSO_UDP_L4 and the aggregate is plain
UDP_L4 from UDP GRO forwarding, could the device get a GSO skb whose
transport offset points at the inner IP header and has no encapsulation
flag? Could it then send malformed segments?

Before this patch these DF aggregates were dropped up front here. The
ip_encap() GSO problem already existed for non-DF GSO skbs, but this
change lets DF traffic reach it as well.

Would it work to apply the exemption only when encap == 0? Other options
are to segment before ip_encap(), or to have ip_encap() set up the
IPXIP4 GSO state the way iptunnel_handle_offloads() does.

[Severity: Low]
The old check was additive and could not wrap:

	if (skb->len+encap > dst4_mtu(&rt->dst) && ...

The new code does an unsigned subtraction instead. On the VIFF_TUNNEL
path encap is sizeof(struct iphdr), which is 20. If the route to
vif->remote has an MTU below 20, can mtu wrap to a value near UINT_MAX?

ip_metrics_convert() only clamps RTAX_MTU from above:

	if (type == RTAX_MTU && val > 65535 - 15)
		val = 65535 - 15;

ip_dst_mtu_maybe_forward() also returns the metric with no lower bound,
apart from subtracting the lwtunnel headroom.

If mtu wraps, skb->len > mtu is false for every DF packet, GSO or not,
so the drop and its IPSTATS_MIB_FRAGFAILS count are skipped. The packet
then gets vif and device tx accounting and goes through the
NF_INET_FORWARD hook. It is only dropped later, by the mtu < hlen + 8
check in ip_do_fragment(), or during GSO segmentation.

This needs a privileged and odd route configuration. Would keeping the
additive comparison, or checking that dst4_mtu() > encap before
subtracting, avoid it?

>  		/* 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/20261010131842.2157365-1-julius%40bairaktaris.de