Re: [PATCH net-next 02/15] net: skbuff: add skb_checksum_full() that reports failure

[netdev-bot+sashiko@xxxxxxxxxx]

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

Critical: 0 · High: 0 · Medium: 1 · 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).

- [Medium] The commit message says the skb_checksum() wrapper
  (net/core/skbuff.c, lines 3657-3664) "behaves as before for the…
- [Low] The new kernel-doc for skb_checksum_full() (net/core/skbuff.c,
  lines 3555-3557) says it returns -EFAULT if "@skb has unreadable frags…

--

net: skbuff: add skb_checksum_full() that reports failure

This adds skb_checksum_full(). It returns -EFAULT for unreadable frags or a
range past the end of the skb, and -EINVAL for a negative length, instead
of returning a 0 checksum. skb_checksum() becomes a wrapper around it and
keeps the old return-0-on-failure behaviour for callers that have not been
converted yet.

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index a2b52a5a61a9..061c61422a38 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3545,12 +3545,31 @@ int skb_store_bits(struct sk_buff *skb, int offset, 
> const void *from, int len)
>  EXPORT_SYMBOL(skb_store_bits);
>  
>  /* Checksum skb data. */
> -__wsum skb_checksum(const struct sk_buff *skb, int offset, int len, __wsum 
> csum)
> +/**
> + *   skb_checksum_full - checksum a range of an skb
> + *   @skb: buffer to checksum
> + *   @offset: offset in @skb to start at
> + *   @len: number of bytes to checksum
> + *   @csump: the sum to add to; on success the range's sum is added to it
> + *
> + *   Return: 0 on success, -EINVAL if @len is negative, or -EFAULT if @skb
> + *   has unreadable frags in the range or runs out before @len does.  On
> + *   failure *@csump is not changed.
> + */

[ ... ]

> @@ -3558,13 +3577,13 @@ __wsum skb_checksum(const struct sk_buff *skb, int 
> offset, int len, __wsum csum)
>                       copy = len;
>               csum = csum_partial(skb->data + offset, copy, csum);
>               if ((len -= copy) == 0)
> -                     return csum;
> +                     goto out;
>               offset += copy;
>               pos     = copy;
>       }
>  
> -     if (WARN_ON_ONCE(!skb_frags_readable(skb)))
> -             return 0;
> +     if (!skb_frags_readable(skb))
> +             return -EFAULT;

[Severity: Low]
This isn't a bug, but does the new kernel-doc match this check? The doc
says -EFAULT is returned when @skb "has unreadable frags in the range".
skb_frags_readable() only tests the skb-wide skb->unreadable bit, though,
and this check runs before working out which frags or frag_list members
the range touches.

That leads to two cases:

- On an unreadable skb, a zero-length range at offset >= skb_headlen()
  returns -EFAULT even though no bytes were requested.

- A range that lies entirely in readable frag_list members, past the head's
  own unreadable frags, is rejected without those members being visited.

The old skb_checksum() behaved the same way here. None of the callers
converted later in the series seem to pass such ranges.

Should the kernel-doc describe the skb-wide check? Or should the check move
so it only applies when a page frag is actually in the range?

[ ... ]

> @@ -3606,14 +3625,17 @@ __wsum skb_checksum(const struct sk_buff *skb, int 
> offset, int len, __wsum csum)
>  
>               end = start + frag_iter->len;
>               if ((copy = end - offset) > 0) {
> -                     __wsum csum2;
> +                     __wsum csum2 = 0;
> +
>                       if (copy > len)
>                               copy = len;
> -                     csum2 = skb_checksum(frag_iter, offset - start, copy,
> -                                          0);
> +                     err = skb_checksum_full(frag_iter, offset - start, copy,
> +                                             &csum2);
> +                     if (err)
> +                             return err;

[ ... ]

> @@ -3621,9 +3643,23 @@ __wsum skb_checksum(const struct sk_buff *skb, int 
> offset, int len, __wsum csum)

[ ... ]

> +/* skb_checksum() has no way to report failure, so it returns 0, which is a
> + * valid checksum.  Use skb_checksum_full() in new code.
> + */
> +__wsum skb_checksum(const struct sk_buff *skb, int offset, int len, __wsum 
> csum)
> +{
> +     if (unlikely(skb_checksum_full(skb, offset, len, &csum))) {
> +             WARN_ON_ONCE(!skb_frags_readable(skb));
> +             return 0;
> +     }

[Severity: Medium]
The commit message says the wrapper "behaves as before for the callers
that haven't been converted: it returns 0 on failure and warns once if
that was because of unreadable frags". Does that hold when the unreadable
frags are in a frag_list member and not in the top-level skb?

Before this patch, the frag_list loop recursed through skb_checksum(). An
unreadable member hit the WARN_ON_ONCE() and returned 0. The parent then
folded that 0 in with csum_block_add() and returned the partial sum of the
readable parts.

Now the member returns -EFAULT from skb_checksum_full() without a warning.
The frag_list loop passes it straight up, and the wrapper only checks the
outer skb:

    WARN_ON_ONCE(!skb_frags_readable(skb));

If the head is readable and only a member is unreadable, the warning is
lost. Unconverted callers also get 0 for the whole range instead of the
old partial sum.

At this revision __skb_checksum_complete() still does:

    csum = skb_checksum(skb, 0, skb->len, 0);

    sum = csum_fold(csum_add(skb->csum, csum));

Could this pass validation whenever the pseudo-header sum folds to 0xffff,
and then set skb->csum to 0 and csum_valid to 1?

The later patch "net: skbuff: fail checksum validation when the skb can't
be checksummed" converts that function. Other skb_checksum() callers stay
unconverted at the end of the series and still see the changed value, for
example net/netfilter/utils.c, nf_nat_proto.c, ipvs, icmp.c, ipmr.c,
udp.c, mptcp and some drivers. The wrapper itself also never starts
warning for nested failures.

Separately, if the top-level skb is unreadable and the failure is the new
-EINVAL for a negative @len, won't this WARN_ON_ONCE() blame unreadable
frags for it?

This mixed layout looks structurally possible. skb_gro_receive() links
skbs onto frag_list without comparing their unreadable flags, and
tcp_gro_receive() doesn't compare them either. It hasn't been shown to
happen in practice.

Should the wrapper warn on nested -EFAULT as well, or should the commit
message be adjusted to describe the new behaviour?

>       return csum;
>  }
>  EXPORT_SYMBOL(skb_checksum);

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