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

[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 new kernel-doc for skb_crc32c_full() (net/core/skbuff.c,
  above line 3810) says it returns -EFAULT when "@skb has unreadable…
- [Low] The commit message says skb_crc32c() 'becomes a wrapper that
  behaves as before', but its unreadable-frags WARN now covers less.

--

net: skbuff: add skb_crc32c_full() that reports failure

This commit adds skb_crc32c_full(). When the walk runs into unreadable
frags or past the end of the skb, it returns an error instead of a CRC
of 0. skb_crc32c() becomes a wrapper around it, and
skb_crc32c_csum_help() is converted, so SCTP packets that can't be
checksummed are dropped instead of being sent with a bogus CRC.

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index de433d452939..3b2a3747f009 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3788,23 +3788,41 @@ int skb_copy_and_csum_bits(const struct sk_buff *skb, 
> int offset,
>  EXPORT_SYMBOL(skb_copy_and_csum_bits);
>  
>  #ifdef CONFIG_NET_CRC32C
> -u32 skb_crc32c(const struct sk_buff *skb, int offset, int len, u32 crc)
> +/**
> + *   skb_crc32c_full - CRC32C a range of an skb
> + *   @skb: buffer to read
> + *   @offset: offset in @skb to start at
> + *   @len: number of bytes to include
> + *   @crcp: the CRC to continue from; on success, updated with the range
> + *
> + *   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 *@crcp is not changed.
> + */
> +int skb_crc32c_full(const struct sk_buff *skb, int offset, int len, u32 
> *crcp)
>  {

[ ... ]

>       if (copy > 0) {
>               copy = min(copy, len);
>               crc = crc32c(crc, skb->data + offset, copy);
>               len -= copy;
>               if (len == 0)
> -                     return crc;
> +                     goto out;
>               offset += copy;
>       }
>  
> -     if (WARN_ON_ONCE(!skb_frags_readable(skb)))
> -             return 0;
> +     if (!skb_frags_readable(skb))
> +             return -EFAULT;

[Severity: Low]
The kernel-doc above says -EFAULT means "@skb has unreadable frags in
the range". Does this check actually look at the range?

skb_frags_readable() only tests the flag for the whole skb:

include/linux/skbuff.h:skb_frags_readable() {
        return !skb->unreadable;
}

Take len == 0 with offset >= skb_headlen(skb). A readable skb walks
zero bytes and returns 0, but an unreadable skb returns -EFAULT here.

Now take a range that lies entirely in readable frag_list children of
an skb whose own frags are unreadable. That also fails here, before the
frag_list loop runs.

No current caller seems to hit this. skb_crc32c_csum_help() always
starts inside the linear head and passes a positive len.

Could the kernel-doc be reworded? Alternatively, this check could apply
only when the remaining range reaches frags[]. skb_checksum_full(),
added earlier in the series, has the same mismatch.

[ ... ]

> @@ -3844,19 +3862,36 @@ u32 skb_crc32c(const struct sk_buff *skb, int offset, 
> int len, u32 crc)
>               copy = end - offset;
>               if (copy > 0) {
>                       copy = min(copy, len);
> -                     crc = skb_crc32c(frag_iter, offset - start, copy, crc);
> +                     err = skb_crc32c_full(frag_iter, offset - start, copy,
> +                                           &crc);
> +                     if (err)
> +                             return err;

[ ... ]

> +/* skb_crc32c() has no way to report failure, so it returns 0, which is a
> + * valid CRC.  Use skb_crc32c_full() in new code.
> + */
> +u32 skb_crc32c(const struct sk_buff *skb, int offset, int len, u32 crc)
> +{
> +     if (unlikely(skb_crc32c_full(skb, offset, len, &crc))) {
> +             WARN_ON_ONCE(!skb_frags_readable(skb));

[Severity: Low]
The commit message says skb_crc32c() "becomes a wrapper that behaves as
before". Does the unreadable-frags warning still fire for frag_list
children?

Before this patch, the frag_list loop recursed through
skb_crc32c(frag_iter, ...). An unreadable child therefore hit
WARN_ON_ONCE(!skb_frags_readable(skb)) at the child level.

Now the recursion goes through skb_crc32c_full(frag_iter, ...). The
child returns -EFAULT without a warning, and this WARN_ON_ONCE() only
tests the outer skb.

So a readable parent with an unreadable frag_list child now returns 0
to sctp_compute_cksum(), siw_crc_skb() or batadv_bla_check_duplist()
without warning. Before the patch it warned once.

The return value is 0 either way, so only the diagnostic changes.
skb_gro_receive() doesn't seem to check readability when it merges into
frag_list, so this layout looks possible, but that hasn't been confirmed
end to end.

The skb_checksum() wrapper from earlier in the series has the same
pattern.

> +             return 0;
> +     }
>       return crc;
>  }
>  EXPORT_SYMBOL(skb_crc32c);

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