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

[David Laight <david.laight.linux@gmail.com>]

On Thu, 22 Oct 2026 10:21:26 +0200
Paolo Abeni <pabeni@redhat.com> wrote:

> 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@kernel.org/
> > Suggested-by: Mina Almasry <almasrymina@google.com>
> > Reviewed-by: Mina Almasry <almasrymina@google.com>
> > Assisted-by: LLM
> > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> > ---
> >   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.

And, as soon as you take &csum, not only does the value have to get
re-read from stack, but the compiles has to assume that the address might
have been saved and can be updated by any other function call.
> 
> 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 could make sense.

That gets really horrid especially on 32bit.

That is why I suggested earlier that since csum values 0 and ~0 are
equivalent one of the two values could be used as an error result.
An 'adc loop' won't normally generate zero, but there might be some
code that relies on it - so would need changing before zero could
be used as an error return value.

David

> 
> 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
>