Re: [PATCH net-next 5/9] net: skbuff: don't BUG() on leftover length in skb_checksum() and friends

[netdev-bot+sashiko@kernel.org]

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 skb_to_sgvec() kernel-doc in net/core/skbuff.c still says it
  "Returns either the number of scatterlist items used, or -EMSGSIZE if…

--

net: skbuff: don't BUG() on leftover length in skb_checksum() and friends

skb_checksum(), skb_crc32c() and __skb_to_sgvec() used to BUG() when
they ran out of skb data before @len was used up. This patch changes
each of them to warn once and return an error instead. __skb_to_sgvec()
now returns -EINVAL, and the two checksum helpers return 0.

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 6cd7135e0dce2..4070e0c25f63e 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c

[ ... ]

> @@ -5333,7 +5335,8 @@ __skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len,
>  		}
>  		start = end;
>  	}
> -	BUG_ON(len);
> +	if (WARN_ON_ONCE(len))
> +		return -EINVAL;

[Severity: Low]
Should the skb_to_sgvec() kernel-doc be updated to match this new return
value? It still lists -EMSGSIZE as the only error:

net/core/skbuff.c:
 *	Fill the specified scatter-gather list with mappings/pointers into a
 *	region of the buffer space attached to a socket buffer. Returns either
 *	the number of scatterlist items used, or -EMSGSIZE if the contents
 *	could not fit.
 */
int skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
{
	int nsg = __skb_to_sgvec(skb, sg, offset, len, 0);

	if (nsg <= 0)
		return nsg;

This -EINVAL is also passed up through the frag_list recursion by
"if (unlikely(ret < 0)) return ret;". skb_to_sgvec() and
skb_to_sgvec_nomark() then return it to their callers unchanged.

All in-tree callers check for any negative return, so nothing breaks
today. However, a caller written to the documented contract that only
checks for -EMSGSIZE would miss the new error.

The comment is still unchanged at the end of the series.

>  	return elt;
>  }
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261006-b4-skbuff-bug-on-v1-0-1b4434c5357c%40toxicpanda.com