Re: [PATCH net-next 05/15] net: skbuff: fail pskb_trim_rcsum() when it can't checksum the tail

[netdev-bot+sashiko@xxxxxxxxxx]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit fixes a real CHECKSUM_COMPLETE corruption in
  pskb_trim_rcsum_complete() (net/core/skbuff.c) but has no Fixes: tag,…
- [Low] In pskb_trim_rcsum_complete() (net/core/skbuff.c), the readable-
  head branch now returns skb_checksum_full()'s -EFAULT unconditionally.

--

net: skbuff: fail pskb_trim_rcsum() when it can't checksum the tail

This patch changes pskb_trim_rcsum_complete() to use skb_checksum_full()
instead of skb_checksum(). If the checksum of the trimmed tail can't be
computed, the error is now returned and callers drop the packet. Before
this, 0 was subtracted from skb->csum.

> pskb_trim_rcsum() can already fail, and its callers drop the packet
> when it does.  Use skb_checksum_full() and return its error.

[Severity: Low]
This isn't a bug, but should this commit carry a Fixes: tag?

Before this patch, the readable-head branch of pskb_trim_rcsum_complete()
only checked the top-level skb->unreadable bit. If the trimmed tail reached
an unreadable frag_list member, skb->csum was left covering the trimmed
bytes.

That layout looks reachable. inet_frag_reasm_finish() keeps
CHECKSUM_COMPLETE, and it links fragments onto frag_list when
skb_try_coalesce() refuses them:

net/core/skbuff.c:skb_try_coalesce() {
    ...
        if (skb_frags_readable(from) != skb_frags_readable(to))
                return false;
    ...
}

The gap seems to go back to 65249feb6b3d ("net: add support for skbs with
unreadable frags"). 2e357f002c61 ("net: Avoid checksumming unreadable skb
tail on trim") only covered the top-level case.

The commit message also mentions the "past the end of the skb" case. That
case was a BUG() until 20ddd0fd2fcd turned it into a silent 0.

Two sibling patches in this series fix the same consequence of
20ddd0fd2fcd, and both carry Fixes: 20ddd0fd2fcd:

  "net: skbuff: fail checksum validation when the skb can't be checksummed"
  "net: skbuff: add skb_crc32c_full() that reports failure"

The commit message also doesn't say that this patch depends on the earlier
"net: skbuff: add skb_checksum_full() that reports failure" patch. Could
that dependency be noted for anyone backporting this?

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 88ed2dd99dc0..de433d452939 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -2829,9 +2829,13 @@ static int pskb_trim_rcsum_complete(struct sk_buff 
> *skb, unsigned int len)
>       int delta = skb->len - len;
>  
>       if (skb_frags_readable(skb)) {
> -             skb->csum = csum_block_sub(skb->csum,
> -                                        skb_checksum(skb, len, delta, 0),
> -                                        len);
> +             __wsum csum = 0;
> +             int err;
> +
> +             err = skb_checksum_full(skb, len, delta, &csum);
> +             if (err)
> +                     return err;

[Severity: Low]
Should this fall back to CHECKSUM_NONE when only the trimmed bytes are
unreadable?

Take a head with skb->unreadable clear, where the tail [len, skb->len) lies
entirely in an unreadable frag_list member. skb_checksum_full() recurses
into that member and returns -EFAULT, and the packet is dropped.

pskb_trim_rcsum_complete() already handles the case where the trimmed bytes
are unreadable but the rest can be checksummed. It only does so when the
top-level bit is set:

        if (len > skb_headlen(skb))
                return -EFAULT;

        /* The trimmed bytes are unreadable, but the remaining packet can be
         * checksummed by software after trimming.
         */
        skb->ip_summed = CHECKSUM_NONE;
        return 0;

So the same data layout is either kept or dropped, depending on whether the
unreadable flag is on the head or on a frag_list member.

One way to get there is IP reassembly on a devmem/zcrx queue. If a readable
first fragment is followed by an unreadable one, inet_frag_reasm_finish()
produces a readable CHECKSUM_COMPLETE head with an unreadable frag_list
member. When the UDP length is shorter than the datagram, the call chain
is:

__udp4_lib_rcv()
  pskb_trim_rcsum(skb, ulen)
    pskb_trim_rcsum_slow()
      pskb_trim_rcsum_complete()
        skb_checksum_full() -> -EFAULT

UDP then drops the packet through short_packet.

___pskb_trim() would have dropped that member and cleared the unreadable
bit:

        if (!skb_shinfo(skb)->nr_frags && !skb_has_frag_list(skb))
                skb->unreadable = 0;

That would leave the trimmed skb fully readable, so software could
checksum it.

Before this patch the packet was accepted. __skb_checksum_complete()
recomputed the checksum over the trimmed skb and reported
netdev_rx_csum_fault().

Would falling back to CHECKSUM_NONE when [0, len) is readable avoid both
the drop and the old splat?

[ ... ]

-- 
Sashiko AI review · 
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261019-b4-skbuff-bug-on-followups-v1-0-41dbef50b9f7%40toxicpanda.com