Re: [PATCH net v5 4/4] net: ipv6: Clamp to IP6_MAX_MTU in ip6_dst_mtu_maybe_forward

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] Incomplete fix: the clamp value chosen (IP6_MAX_MTU = 0xFFFF +
  sizeof(struct ipv6hdr) = 65575, include/net/ip6_route.h:44) is 40…

--

net: ipv6: Clamp to IP6_MAX_MTU in ip6_dst_mtu_maybe_forward

This restores an IP6_MAX_MTU upper bound on the value returned by
ip6_dst_mtu_maybe_forward(), which was dropped when the helper was
introduced in commit 427faee167bc. The stated goal is to avoid 16-bit
overflows in forwarding paths, mirroring the IPv4 clamp added by commit
ac6627a28dbf.

> diff --git a/include/net/ip6_route.h b/include/net/ip6_route.h
> index c69f1c8719223..b9e8d2b759e9b 100644
> --- a/include/net/ip6_route.h
> +++ b/include/net/ip6_route.h
> @@ -384,6 +384,8 @@ static inline unsigned int ip6_dst_mtu_maybe_forward(const struct dst_entry *dst
>  	rcu_read_unlock();
>  
>  out:
> +	mtu = min_t(unsigned int, mtu, IP6_MAX_MTU);
> +

[Severity: Medium]
Does this clamp actually bound the 16-bit store the changelog refers to?

IP6_MAX_MTU in this same header is:

include/net/ip6_route.h:
	#define IP6_MAX_MTU (0xFFFF + sizeof(struct ipv6hdr))

so the bound is 65575, which is 40 above U16_MAX.

The IPv4 precedent quoted in the commit message does not look analogous
here.  include/linux/ip.h has:

	/* IPv4 datagram length is stored into 16bit field (tot_len) */
	#define IP_MAX_MTU	0xFFFFU

That is exactly U16_MAX, so the IPv4 clamp does bound a u16 consumer, while
the IPv6 value chosen here does not.

The forwarding-path consumer that narrows to 16 bits is
flow_offload_fill_route() in net/netfilter/nf_flow_table_core.c:

	case NFPROTO_IPV6:
		flow_tuple->mtu = ip6_dst_mtu_maybe_forward(dst, true);

and the destination field in include/net/netfilter/nf_flow_table.h is:

	struct flow_offload_tuple {
		...
		u16				mtu;

Any returned value in [65536, 65575] truncates to 0..39 there.

Is the new deterministic output worse than the unclamped case for that
consumer?  After this change every egress device with an MTU at or above
65575 yields exactly 65575, which truncates to exactly 39.  Then in
nf_flow_offload_ipv6_forward() in net/netfilter/nf_flow_table_ip.c:

	mtu = flow->tuplehash[dir].tuple.mtu + ctx->offset;
	if (flow->tuplehash[!dir].tuple.tun_num)
		mtu -= sizeof(*ip6h);

	if (unlikely(nf_flow_exceeds_mtu(skb, mtu)))
		return 0;

With a truncated tuple.mtu of 39 and the peer direction carrying tun_num,
the unsigned int subtraction of sizeof(struct ipv6hdr) wraps to roughly
4294967295, and nf_flow_exceeds_mtu():

	static bool nf_flow_exceeds_mtu(const struct sk_buff *skb, unsigned int mtu)
	{
		if (skb->len <= mtu)
			return false;

then returns false for every packet size, so the fast-path MTU check no
longer fires.

Is a raw MTU above U16_MAX reachable in-tree?  The idev->cnf.mtu6 branch
takes dev->mtu, and vrf_setup() in drivers/net/vrf.c does:

	dev->min_mtu = IPV6_MIN_MTU;
	dev->max_mtu = IP6_MAX_MTU;
	dev->mtu = dev->max_mtu;

Devices that leave max_mtu unset (loopback, for instance) also accept
arbitrarily large values, since dev_validate_mtu() in net/core/dev.c only
enforces an upper bound when max_mtu is non-zero:

	if (dev->max_mtu > 0 && new_mtu > dev->max_mtu) {

Would clamping to U16_MAX, or fixing the narrowing at the flowtable
consumer, be closer to what the changelog describes?

One related note on the reasoning: the locked route metric path does not
appear to be a way to get above 65535, since ip_metrics_convert() in
net/ipv4/metrics.c clamps RTAX_MTU to 65520 and is the only writer for
user-supplied IPv6 route metrics.

>  	return mtu - lwtunnel_headroom(dst->lwtstate, mtu);
>  }
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260901195714.673548-1-alice.kernel%40fastmail.im