Re: [PATCH net v3] ipv6: fix dead half of the MTU guard in __ip6_append_data()

[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

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] In __ip6_append_data() (net/ipv6/ip6_output.c:1502-1507), the
  patch adds a new minimum check: `mtu < reserved_headerlen +…

--

ipv6: fix dead half of the MTU guard in __ip6_append_data()

This patch replaces the MTU entry guard in __ip6_append_data(). The
second half of the old guard could never be true. The new guard requires
room for the fragmentable headers, plus dst.header_len for non-tunnel
xfrm, plus a fragment header and one 8-octet payload block. The goal is
to stop maxfraglen from underflowing, which leads to an skb_over_panic()
in skb_put().

> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> index 25fe0ba98b1a6..936dad85f1c98 100644
> --- a/net/ipv6/ip6_output.c
> +++ b/net/ipv6/ip6_output.c

[ ... ]

> @@ -1499,8 +1499,11 @@ static int __ip6_append_data(struct sock *sk,
>  		     (opt ? opt->opt_flen + opt->opt_nflen : 0) +
>  		     rt->rt6i_nfheader_len;
>  
> -	if (mtu <= fragheaderlen ||
> -	    ((mtu - fragheaderlen) & ~7) + fragheaderlen <= sizeof(struct frag_hdr))
> +	reserved_headerlen = fragheaderlen;
> +	if (!skb && !(rt->dst.flags & DST_XFRM_TUNNEL))
> +		reserved_headerlen += rt->dst.header_len;
> +
> +	if (mtu < reserved_headerlen + sizeof(struct frag_hdr) + 8)
>  		goto emsgsize;

[Severity: Medium]
Does this new check also reject datagrams that fit in the MTU and would
never be fragmented?

The minimum is computed only from header sizes. It runs before
__ip6_append_data() knows whether this send needs fragmenting. Take an
IPv6 socket whose extension headers (for example a hop-by-hop options
cmsg) leave mtu - fragheaderlen between 1 and 15, and a payload where
headersize + transhdrlen + length <= mtu. That send used to work.

On the first skb, the alloc_new_skb path picks the mtu side of the
clamp, because cork->length <= mtu:

    if (datalen > (cork->length <= mtu ? mtu : maxfraglen) - fragheaderlen)
        datalen = maxfraglen - fragheaderlen - rt->dst.trailer_len;

The clamp does not fire, so fraglen = fragheaderlen + length <= mtu. The
underflowed maxfraglen is never used on this path. After that,
__ip6_finish_output() sends the packet without fragmenting it:

    if (unlikely(skb->len > mtu ||
        (IP6CB(skb)->frag_max_size && skb->len > IP6CB(skb)->frag_max_size)))
        return ip6_fragment(net, sk, skb, ip6_finish_output2);

With this patch, the same sendmsg() returns -EMSGSIZE from the new check,
and ipv6_local_error() reports a pmtu.

The commit message says:

    Replace the dead expression with a single check that requires room
    for at least one 8-octet payload block after the fragment header,
    matching the minimum ip6_fragment() already enforces:

ip6_fragment() only applies this minimum to packets that are larger than
the MTU and are actually being fragmented:

    if (mtu < hlen + sizeof(struct frag_hdr) + 8)
        goto fail_toobig;

Here it applies to every send, including ones that fit. Since IPV6_MTU
accepts any value >= 1280, it is not hard to land in that window. The
patch is also tagged for stable, so stable trees would get this change
in behaviour that userspace can see.

Could the minimum be applied only when this call will need to fragment?
For example, gate it on cork->length + length > mtu - headersize,
evaluated before cork->length += length. Or could the commit message at
least describe the change for unfragmented sends?

>  
>  	maxfraglen = ((mtu - fragheaderlen) & ~7) + fragheaderlen -

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/tencent_AE3DA7423209553B562E5C7FBCB808EB680A%40qq.com