Re: [PATCH net-next v2 01/15] net: skbuff: make skb_copy_and_csum_bits() report unreadable frags

[Paolo Abeni <pabeni@xxxxxxxxxx>]

On 10/21/26 15:46, Josef Bacik wrote:
> skb_copy_and_csum_bits() has no way to fail.  When it reaches
> unreadable frags it zero-fills the rest of the buffer and returns 0,
> and since commit ab9414ed70bd ("net: skbuff: don't leave stale bytes in
> skb_copy_and_csum_bits()") that 0 is the correct checksum for what's in
> the buffer.  Its callers can't tell the copy didn't happen.
>
> That matters in the !sg && !csum branch of skb_segment().  A devmem TCP
> skb keeps its whole payload in unreadable frags, so every segment ends
> up with a payload of zeroes, tcp_gso_segment() writes a valid TCP
> checksum over it, and the peer accepts the zeroes as stream data.  This
> is reachable when both SG and TX checksum offload are turned off on a
> device used for devmem TX: sk_setup_caps() still gives TCP SG and
> HW_CSUM, so the dmabuf send is accepted, validate_xmit_unreadable_skb()
> lets the skb through for a NETMEM_TX_NO_DMA device or a matching
> binding, and software GSO segments it with neither feature.  Before
> that commit the segments carried uninitialized memory with a checksum
> that almost never matched.
>
> This was reproduced with a test module rather than on devmem hardware:
> skb_segment() on a TCPv4 GSO skb with 3000 bytes of payload in a page
> frag marked unreadable, with features == 0, returns 3 segments whose
> payloads are all zeroes, each with a checksum of 0, which is correct for
> zeroes.  There's no warning or error; the data is silently replaced.
> With this patch skb_segment() returns -EFAULT.
>
> Make skb_copy_and_csum_bits() return 0 or -EFAULT and hand the checksum
> back through a pointer, and make it __must_check so a new caller can't
> ignore the failure.  Running out of skb before @len returns -EFAULT
> too; commit 417261779eab ("net: skbuff: don't BUG() on leftover length
> in skb_copy_and_csum_bits()") made that a zero-fill and a checksum of 0.
> Each caller takes the error path it already has for a failed copy:
>
>   - skb_segment() fails the segmentation, as the csum branch already
>     does when skb_copy_bits() fails.
>   - icmp_glue_bits() and icmpv6_getfrag() return the error, so the ICMP
>     error isn't sent.
>   - __ip_append_data() and __ip6_append_data() drop the new skb and
>     return -EFAULT, as they do when getfrag fails.
>   - vnet_skb_shape() fails the same way it does when its skb_copy_bits()
>     call fails.
>   - xdr_skb_read_bits() returns 0 without consuming anything, as it does
>     when skb_copy_bits() fails, so desc.count stays non-zero and
>     csum_partial_copy_to_xdr() fails.  csum_partial_copy_to_xdr() used
>     to checksum the bytes it hadn't copied before checking desc.count.
>     desc.offset and desc.count always add up to skb->len, so that only
>     ran when it was about to fail anyway, and on an unreadable skb
>     skb_checksum() would WARN.  Drop it.
>   - skb_copy_and_csum_dev() can't return an error, so it leaves the
>     checksum alone and the frame goes out with a bad one.  Its callers
>     are drivers that never see unreadable skbs anyway.
>
> The zero-fill stays, so a caller that does send the buffer still
> doesn't leak anything.
>
> This was found by the Sashiko AI review of that commit, and
> Mina suggested reporting the error from the helper rather than checking
> for unreadable frags in skb_segment().
>
> Link: 
> https://lore.kernel.org/all/179148235783.434549.14322374227477832817@xxxxxxxxxx/
> Suggested-by: Mina Almasry <almasrymina@xxxxxxxxxx>
> Reviewed-by: Mina Almasry <almasrymina@xxxxxxxxxx>
> Assisted-by: LLM
> Signed-off-by: Josef Bacik <josef@xxxxxxxxxxxxxx>
> ---
>   drivers/net/ethernet/sun/sunvnet_common.c | 10 +++--
>   include/linux/skbuff.h                    |  4 +-
>   net/core/skbuff.c                         | 63 +++++++++++++++++++++----------
>   net/ipv4/icmp.c                           |  9 +++--
>   net/ipv4/ip_output.c                      | 11 ++++--
>   net/ipv6/icmp.c                           |  7 +++-
>   net/ipv6/ip6_output.c                     | 11 ++++--
>   net/sunrpc/socklib.c                      |  9 ++---
>   8 files changed, 83 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/net/ethernet/sun/sunvnet_common.c 
> b/drivers/net/ethernet/sun/sunvnet_common.c
> index 5cca95869052..4f229fcecc32 100644
> --- a/drivers/net/ethernet/sun/sunvnet_common.c
> +++ b/drivers/net/ethernet/sun/sunvnet_common.c
> @@ -1167,9 +1167,13 @@ static inline struct sk_buff *vnet_skb_shape(struct 
> sk_buff *skb, int ncookies)
>   
>   			/* copy the rest, with csum calculation */
>                         *(__sum16 *)(skb->data + offset) = 0;
> -                       csum = skb_copy_and_csum_bits(skb, start,
> -                                                     nskb->data + start,
> -                                                     skb->len - start);
> +                       if (skb_copy_and_csum_bits(skb, start,
> +                                                  nskb->data + start,
> +                                                  skb->len - start, &csum)) {
Recently Eric did some work to remove return arguments from the fast-path,
as with some config it gives measurable overhead. See commit
a02a765bd5c2ae7705144829b2911c96c29db6ba.

S/W csum should not be the fastest path, but AFAICS the UDP output path
could use it when building the aggregate segments.

I'm wondering using a 64 bit return value and encoding there both the csum
and the error coudl make sense.

Also currently net-next is closed for the merge window, and a bunch of
changes in this series looks more like cleanup than fixes. Cleanup will have
to wait for the end of the merge window, fixes can land at any time.

/P