All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net v2] net: skbuff: make skb_copy_and_csum_bits() report unreadable frags
@ 2026-10-13 18:02 Josef Bacik
  2026-10-13 18:05 ` netdev-bot+sinfo
  2026-10-14 12:51 ` David Laight
  0 siblings, 2 replies; 9+ messages in thread
From: Josef Bacik @ 2026-10-13 18:02 UTC (permalink / raw)
  To: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Simon Horman, David Ahern, Ido Schimmel,
	Trond Myklebust, Anna Schumaker, Chuck Lever, Jeff Layton,
	NeilBrown, Olga Kornievskaia, Dai Ngo, Tom Talpey, Josef Bacik,
	Mina Almasry, Kaiyuan Zhang, Willem de Bruijn
  Cc: netdev, linux-kernel, linux-nfs, stable

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.

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.  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().

Fixes: ab9414ed70bd ("net: skbuff: don't leave stale bytes in skb_copy_and_csum_bits()")
Fixes: 65249feb6b3d ("net: add support for skbs with unreadable frags")
Link: https://lore.kernel.org/all/179148235783.434549.14322374227477832817@kernel.org/
Suggested-by: Mina Almasry <almasrymina@google.com>
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
v1: https://lore.kernel.org/all/20261009-b4-skb-segment-nosg-unreadable-v1-1-7e2334a55fe8@toxicpanda.com/

v1->v2:
- Report the failure from skb_copy_and_csum_bits() itself and handle it
  in every caller, instead of checking for unreadable frags up front in
  skb_segment() (Mina).
- Rebased onto current net.

Thanks,
Josef
---
 drivers/net/ethernet/sun/sunvnet_common.c | 10 +++--
 include/linux/skbuff.h                    |  4 +-
 net/core/skbuff.c                         | 61 ++++++++++++++++++++++---------
 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, 82 insertions(+), 40 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)) {
+				dev_kfree_skb(nskb);
+				dev_kfree_skb(skb);
+				return NULL;
+			}
 
 			/* add in the header checksums */
 			if (skb->protocol == htons(ETH_P_IP)) {
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index a3ff380d43f0..542494fabbc4 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -4268,8 +4268,8 @@ void skb_free_datagram(struct sock *sk, struct sk_buff *skb);
 int skb_kill_datagram(struct sock *sk, struct sk_buff *skb, unsigned int flags);
 int skb_copy_bits(const struct sk_buff *skb, int offset, void *to, int len);
 int skb_store_bits(struct sk_buff *skb, int offset, const void *from, int len);
-__wsum skb_copy_and_csum_bits(const struct sk_buff *skb, int offset, u8 *to,
-			      int len);
+int __must_check skb_copy_and_csum_bits(const struct sk_buff *skb, int offset,
+					u8 *to, int len, __wsum *csum);
 int skb_splice_bits(struct sk_buff *skb, struct sock *sk, unsigned int offset,
 		    struct pipe_inode_info *pipe, unsigned int len,
 		    unsigned int flags);
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 41beaf625421..9152448324f8 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3611,14 +3611,27 @@ EXPORT_SYMBOL(skb_checksum);
 
 /* Both of above in one bottle. */
 
-__wsum skb_copy_and_csum_bits(const struct sk_buff *skb, int offset,
-				    u8 *to, int len)
+/**
+ *	skb_copy_and_csum_bits - copy data out of an skb and checksum it
+ *	@skb: source buffer
+ *	@offset: offset in @skb to start copying from
+ *	@to: destination buffer
+ *	@len: number of bytes to copy
+ *	@csump: where to store the checksum of the copied bytes
+ *
+ *	Return: 0 on success, or -EFAULT if @skb has unreadable frags.  On
+ *	failure *@csump is not set and the part of @to that couldn't be filled
+ *	is zeroed.
+ */
+int skb_copy_and_csum_bits(const struct sk_buff *skb, int offset,
+			   u8 *to, int len, __wsum *csump)
 {
 	int start = skb_headlen(skb);
 	int i, copy = start - offset;
 	struct sk_buff *frag_iter;
 	int pos = 0;
 	__wsum csum = 0;
+	int err;
 
 	/* Copy header. */
 	if (copy > 0) {
@@ -3627,7 +3640,7 @@ __wsum skb_copy_and_csum_bits(const struct sk_buff *skb, int offset,
 		csum = csum_partial_copy_nocheck(skb->data + offset, to,
 						 copy);
 		if ((len -= copy) == 0)
-			return csum;
+			goto out;
 		offset += copy;
 		to     += copy;
 		pos	= copy;
@@ -3637,7 +3650,7 @@ __wsum skb_copy_and_csum_bits(const struct sk_buff *skb, int offset,
 		/* Don't hand the caller a buffer with stale bytes in it. */
 		if (len > 0)
 			memset(to, 0, len);
-		return 0;
+		return -EFAULT;
 	}
 
 	for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
@@ -3669,7 +3682,7 @@ __wsum skb_copy_and_csum_bits(const struct sk_buff *skb, int offset,
 			}
 
 			if (!(len -= copy))
-				return csum;
+				goto out;
 			offset += copy;
 			to     += copy;
 		}
@@ -3686,12 +3699,16 @@ __wsum skb_copy_and_csum_bits(const struct sk_buff *skb, int offset,
 		if ((copy = end - offset) > 0) {
 			if (copy > len)
 				copy = len;
-			csum2 = skb_copy_and_csum_bits(frag_iter,
-						       offset - start,
-						       to, copy);
+			err = skb_copy_and_csum_bits(frag_iter, offset - start,
+						     to, copy, &csum2);
+			if (err) {
+				/* The child zeroed its part; zero the rest. */
+				memset(to + copy, 0, len - copy);
+				return err;
+			}
 			csum = csum_block_add(csum, csum2, pos);
 			if ((len -= copy) == 0)
-				return csum;
+				goto out;
 			offset += copy;
 			to     += copy;
 			pos    += copy;
@@ -3699,7 +3716,9 @@ __wsum skb_copy_and_csum_bits(const struct sk_buff *skb, int offset,
 		start = end;
 	}
 	BUG_ON(len);
-	return csum;
+out:
+	*csump = csum;
+	return 0;
 }
 EXPORT_SYMBOL(skb_copy_and_csum_bits);
 
@@ -3964,9 +3983,13 @@ void skb_copy_and_csum_dev(const struct sk_buff *skb, u8 *to)
 	skb_copy_from_linear_data(skb, to, csstart);
 
 	csum = 0;
-	if (csstart != skb->len)
-		csum = skb_copy_and_csum_bits(skb, csstart, to + csstart,
-					      skb->len - csstart);
+	/* On failure the rest of @to is zeroed; leave the checksum alone so
+	 * the frame goes out with a bad one.
+	 */
+	if (csstart != skb->len &&
+	    skb_copy_and_csum_bits(skb, csstart, to + csstart,
+				   skb->len - csstart, &csum))
+		return;
 
 	if (skb->ip_summed == CHECKSUM_PARTIAL) {
 		long csstuff = csstart + skb->csum_offset;
@@ -4993,11 +5016,13 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
 			if (!csum) {
 				if (!nskb->remcsum_offload)
 					nskb->ip_summed = CHECKSUM_NONE;
-				SKB_GSO_CB(nskb)->csum =
-					skb_copy_and_csum_bits(head_skb, offset,
-							       skb_put(nskb,
-								       len),
-							       len);
+				if (skb_copy_and_csum_bits(head_skb, offset,
+							   skb_put(nskb, len),
+							   len,
+							   &SKB_GSO_CB(nskb)->csum)) {
+					err = -EFAULT;
+					goto err;
+				}
 				SKB_GSO_CB(nskb)->csum_start =
 					skb_headroom(nskb) + doffset;
 			} else {
diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index 90c0e22c29be..35008785e3d2 100644
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
@@ -364,12 +364,15 @@ static int icmp_glue_bits(void *from, char *to, int offset, int len, int odd,
 	DEFINE_RAW_FLEX(struct icmp_bxm, icmp_param, replyopts.opt.__data,
 			IP_OPTIONS_DATA_FIXED_SIZE);
 	__wsum csum;
+	int err;
 
 	icmp_param = from;
 
-	csum = skb_copy_and_csum_bits(icmp_param->skb,
-				      icmp_param->offset + offset,
-				      to, len);
+	err = skb_copy_and_csum_bits(icmp_param->skb,
+				     icmp_param->offset + offset,
+				     to, len, &csum);
+	if (err)
+		return err;
 
 	skb->csum = csum_block_add(skb->csum, csum, odd);
 	if (icmp_param->data.icmph.type <= NR_ICMP_TYPES &&
diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index eaa6fabda347..e3a927066f07 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -1162,9 +1162,14 @@ static int __ip_append_data(struct sock *sk,
 			data += fragheaderlen + exthdrlen;
 
 			if (fraggap) {
-				skb->csum = skb_copy_and_csum_bits(
-					skb_prev, maxfraglen,
-					data + transhdrlen, fraggap);
+				if (skb_copy_and_csum_bits(skb_prev, maxfraglen,
+							   data + transhdrlen,
+							   fraggap,
+							   &skb->csum)) {
+					err = -EFAULT;
+					kfree_skb(skb);
+					goto error;
+				}
 				skb_prev->csum = csum_sub(skb_prev->csum,
 							  skb->csum);
 				data += fraggap;
diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
index a95b0351824f..31ca3da80b1f 100644
--- a/net/ipv6/icmp.c
+++ b/net/ipv6/icmp.c
@@ -322,9 +322,12 @@ static int icmpv6_getfrag(void *from, char *to, int offset, int len, int odd, st
 	struct icmpv6_msg *msg = (struct icmpv6_msg *) from;
 	struct sk_buff *org_skb = msg->skb;
 	__wsum csum;
+	int err;
 
-	csum = skb_copy_and_csum_bits(org_skb, msg->offset + offset,
-				      to, len);
+	err = skb_copy_and_csum_bits(org_skb, msg->offset + offset,
+				     to, len, &csum);
+	if (err)
+		return err;
 	skb->csum = csum_block_add(skb->csum, csum, odd);
 	if (!(msg->type & ICMPV6_INFOMSG_MASK))
 		nf_ct_attach(skb, org_skb);
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 550965058991..5c297d38eb8c 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -1724,9 +1724,14 @@ static int __ip6_append_data(struct sock *sk,
 			skb->transport_header = (skb->network_header +
 						 fragheaderlen);
 			if (fraggap) {
-				skb->csum = skb_copy_and_csum_bits(
-					skb_prev, maxfraglen,
-					data + transhdrlen, fraggap);
+				if (skb_copy_and_csum_bits(skb_prev, maxfraglen,
+							   data + transhdrlen,
+							   fraggap,
+							   &skb->csum)) {
+					err = -EFAULT;
+					kfree_skb(skb);
+					goto error;
+				}
 				skb_prev->csum = csum_sub(skb_prev->csum,
 							  skb->csum);
 				data += fraggap;
diff --git a/net/sunrpc/socklib.c b/net/sunrpc/socklib.c
index d8d8842c7de5..5d780ad4ae3a 100644
--- a/net/sunrpc/socklib.c
+++ b/net/sunrpc/socklib.c
@@ -49,7 +49,9 @@ xdr_skb_read_bits(struct xdr_skb_reader *desc, void *to, size_t len)
 	if (desc->need_checksum) {
 		__wsum csum;
 
-		csum = skb_copy_and_csum_bits(desc->skb, desc->offset, to, len);
+		if (unlikely(skb_copy_and_csum_bits(desc->skb, desc->offset,
+						    to, len, &csum)))
+			return 0;
 		desc->csum = csum_block_add(desc->csum, csum, desc->offset);
 	} else {
 		if (unlikely(skb_copy_bits(desc->skb, desc->offset, to, len)))
@@ -142,11 +144,6 @@ int csum_partial_copy_to_xdr(struct xdr_buf *xdr, struct sk_buff *skb)
 	desc.csum = csum_partial(skb->data, desc.offset, skb->csum);
 	if (xdr_partial_copy_from_skb(xdr, &desc) < 0)
 		return -1;
-	if (desc.offset != skb->len) {
-		__wsum csum2;
-		csum2 = skb_checksum(skb, desc.offset, skb->len - desc.offset, 0);
-		desc.csum = csum_block_add(desc.csum, csum2, desc.offset);
-	}
 	if (desc.count)
 		return -1;
 	if (csum_fold(desc.csum))

---
base-commit: af32da41b0327b9c6a37856ba82b6760d6c8d10e
change-id: 20261008-b4-skb-segment-nosg-unreadable-aa527ce1b14f


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH net v2] net: skbuff: make skb_copy_and_csum_bits() report unreadable frags
  2026-10-13 18:02 [PATCH net v2] net: skbuff: make skb_copy_and_csum_bits() report unreadable frags Josef Bacik
@ 2026-10-13 18:05 ` netdev-bot+sinfo
  2026-10-14 14:04   ` Josef Bacik
  2026-10-14 12:51 ` David Laight
  1 sibling, 1 reply; 9+ messages in thread
From: netdev-bot+sinfo @ 2026-10-13 18:05 UTC (permalink / raw)
  To: Josef Bacik
  Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Simon Horman, David Ahern, Ido Schimmel,
	Trond Myklebust, Anna Schumaker, Chuck Lever, Jeff Layton,
	NeilBrown, Olga Kornievskaia, Dai Ngo, Tom Talpey, Mina Almasry,
	Kaiyuan Zhang, Willem de Bruijn, netdev, linux-kernel, linux-nfs,
	stable

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net v2] net: skbuff: make skb_copy_and_csum_bits() report unreadable frags
  2026-10-13 18:02 [PATCH net v2] net: skbuff: make skb_copy_and_csum_bits() report unreadable frags Josef Bacik
  2026-10-13 18:05 ` netdev-bot+sinfo
@ 2026-10-14 12:51 ` David Laight
  2026-10-14 14:04   ` Josef Bacik
  1 sibling, 1 reply; 9+ messages in thread
From: David Laight @ 2026-10-14 12:51 UTC (permalink / raw)
  To: Josef Bacik
  Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Simon Horman, David Ahern, Ido Schimmel,
	Trond Myklebust, Anna Schumaker, Chuck Lever, Jeff Layton,
	NeilBrown, Olga Kornievskaia, Dai Ngo, Tom Talpey, Mina Almasry,
	Kaiyuan Zhang, Willem de Bruijn, netdev, linux-kernel, linux-nfs,
	stable

On Tue, 13 Oct 2026 18:02:44 +0000
Josef Bacik <josef@toxicpanda.com> wrote:

> Subject: [PATCH net v2] net: skbuff: make skb_copy_and_csum_bits() report  unreadable frags
> Date: Tue, 13 Oct 2026 18:02:44 +0000
> 
> 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.

Can't you use that fact that the csum function should only return
either 0 or ~0 and use the other value to indicate an error.

ISTR this coming up before with the 'copy_to/from_user_and_csum' code.

David

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net v2] net: skbuff: make skb_copy_and_csum_bits() report unreadable frags
  2026-10-13 18:05 ` netdev-bot+sinfo
@ 2026-10-14 14:04   ` Josef Bacik
  0 siblings, 0 replies; 9+ messages in thread
From: Josef Bacik @ 2026-10-14 14:04 UTC (permalink / raw)
  To: netdev-bot+sinfo
  Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Simon Horman, Andrew Lunn, David Ahern, Ido Schimmel,
	Trond Myklebust, Anna Schumaker, Chuck Lever, Jeff Layton,
	NeilBrown, Olga Kornievskaia, Dai Ngo, Tom Talpey, Mina Almasry,
	Kaiyuan Zhang, Willem de Bruijn, netdev, linux-kernel, linux-nfs,
	stable

On Tue, Oct 13, 2026 at 06:05:30PM +0000, netdev-bot+sinfo@kernel.org wrote:
>  - Whether the issue was actually triggered, or is only theoretical
>    (e.g. found by code inspection). If it was triggered please include
>    the symptoms, like the stack trace or error messages.

It was found by the Sashiko review of ab9414ed70bd and then triggered
with a test module, not on devmem hardware.  The module builds a TCPv4
GSO skb with 3000 bytes of payload in a page frag marked unreadable and
calls skb_segment() on it with features == 0.  Without this patch that
returns 3 segments whose payloads are all zeroes, each with a
SKB_GSO_CB()->csum of 0, which is the correct checksum for zeroes.
There's no stack trace or error, the data is just silently replaced.
With this patch skb_segment() returns -EFAULT.

Thanks,
Josef

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net v2] net: skbuff: make skb_copy_and_csum_bits() report unreadable frags
  2026-10-14 12:51 ` David Laight
@ 2026-10-14 14:04   ` Josef Bacik
  2026-10-14 16:30     ` David Laight
  0 siblings, 1 reply; 9+ messages in thread
From: Josef Bacik @ 2026-10-14 14:04 UTC (permalink / raw)
  To: David Laight
  Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Simon Horman, Andrew Lunn, David Ahern, Ido Schimmel,
	Trond Myklebust, Anna Schumaker, Chuck Lever, Jeff Layton,
	NeilBrown, Olga Kornievskaia, Dai Ngo, Tom Talpey, Mina Almasry,
	Kaiyuan Zhang, Willem de Bruijn, netdev, linux-kernel, linux-nfs,
	stable

On Wed, Oct 14, 2026 at 01:51:54PM +0100, David Laight wrote:
> Can't you use that fact that the csum function should only return
> either 0 or ~0 and use the other value to indicate an error.
>
> ISTR this coming up before with the 'copy_to/from_user_and_csum' code.

skb_copy_and_csum_bits() returns an unfolded __wsum starting from 0, so
0 is a valid result today.  It would work if it started from ~0 the way
csum_and_copy_from_user() does, since that doesn't change the folded
result and the sum can never come out as 0.

But this is one of those "you have to know the behavior of the function
to use it properly." Explicit errors are always better, it removes
ambiguity and creates cleaner and less error prone code. I'm happy to
change it to start with ~0 as well to add the extra protection, but
explicit errors are always best. Thanks,

Josef

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net v2] net: skbuff: make skb_copy_and_csum_bits() report unreadable frags
  2026-10-14 14:04   ` Josef Bacik
@ 2026-10-14 16:30     ` David Laight
  2026-10-14 16:52       ` Josef Bacik
  0 siblings, 1 reply; 9+ messages in thread
From: David Laight @ 2026-10-14 16:30 UTC (permalink / raw)
  To: Josef Bacik
  Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Simon Horman, Andrew Lunn, David Ahern, Ido Schimmel,
	Trond Myklebust, Anna Schumaker, Chuck Lever, Jeff Layton,
	NeilBrown, Olga Kornievskaia, Dai Ngo, Tom Talpey, Mina Almasry,
	Kaiyuan Zhang, Willem de Bruijn, netdev, linux-kernel, linux-nfs,
	stable

On Wed, 14 Oct 2026 14:04:45 +0000
Josef Bacik <josef@toxicpanda.com> wrote:

> On Wed, Oct 14, 2026 at 01:51:54PM +0100, David Laight wrote:
> > Can't you use that fact that the csum function should only return
> > either 0 or ~0 and use the other value to indicate an error.
> >
> > ISTR this coming up before with the 'copy_to/from_user_and_csum' code.  
> 
> skb_copy_and_csum_bits() returns an unfolded __wsum starting from 0, so
> 0 is a valid result today.  It would work if it started from ~0 the way
> csum_and_copy_from_user() does, since that doesn't change the folded
> result and the sum can never come out as 0.

The result can only be 0 if either the length is zero or all the bytes
are zero.
You can probably prove that is never required.
It is likely better to initialise the csum to either 0xffff or ~0.

IIRC both IPv4 and IPv6 complement the checksum before sending it.
For IPv6 a zero is invalid (it means checksum not calculated for IPv4),
I think the best was to achieve that is to initialise the csum to 1
and then add 1 after inverting it.

> But this is one of those "you have to know the behavior of the function
> to use it properly." Explicit errors are always better, it removes
> ambiguity and creates cleaner and less error prone code. I'm happy to
> change it to start with ~0 as well to add the extra protection, but
> explicit errors are always best. Thanks,

Look at all the places ERR_PTR() gets used.
An in-band error indication gives better code and is probably less buggy!

David

> 
> Josef


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net v2] net: skbuff: make skb_copy_and_csum_bits() report unreadable frags
  2026-10-14 16:30     ` David Laight
@ 2026-10-14 16:52       ` Josef Bacik
  2026-10-14 17:27         ` Josef Bacik
  0 siblings, 1 reply; 9+ messages in thread
From: Josef Bacik @ 2026-10-14 16:52 UTC (permalink / raw)
  To: David Laight
  Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Simon Horman, Andrew Lunn, David Ahern, Ido Schimmel,
	Trond Myklebust, Anna Schumaker, Chuck Lever, Jeff Layton,
	NeilBrown, Olga Kornievskaia, Dai Ngo, Tom Talpey, Mina Almasry,
	Kaiyuan Zhang, Willem de Bruijn, netdev, linux-kernel, linux-nfs,
	stable

On Wed, Oct 14, 2026 at 12:30 PM David Laight
<david.laight.linux@gmail.com> wrote:
>
> On Wed, 14 Oct 2026 14:04:45 +0000
> Josef Bacik <josef@toxicpanda.com> wrote:
>
> > On Wed, Oct 14, 2026 at 01:51:54PM +0100, David Laight wrote:
> > > Can't you use that fact that the csum function should only return
> > > either 0 or ~0 and use the other value to indicate an error.
> > >
> > > ISTR this coming up before with the 'copy_to/from_user_and_csum' code.
> >
> > skb_copy_and_csum_bits() returns an unfolded __wsum starting from 0, so
> > 0 is a valid result today.  It would work if it started from ~0 the way
> > csum_and_copy_from_user() does, since that doesn't change the folded
> > result and the sum can never come out as 0.
>
> The result can only be 0 if either the length is zero or all the bytes
> are zero.
> You can probably prove that is never required.
> It is likely better to initialise the csum to either 0xffff or ~0.
>
> IIRC both IPv4 and IPv6 complement the checksum before sending it.
> For IPv6 a zero is invalid (it means checksum not calculated for IPv4),
> I think the best was to achieve that is to initialise the csum to 1
> and then add 1 after inverting it.
>
> > But this is one of those "you have to know the behavior of the function
> > to use it properly." Explicit errors are always better, it removes
> > ambiguity and creates cleaner and less error prone code. I'm happy to
> > change it to start with ~0 as well to add the extra protection, but
> > explicit errors are always best. Thanks,
>
> Look at all the places ERR_PTR() gets used.
> An in-band error indication gives better code and is probably less buggy!
>

...ERR_PTR would be fine too? I'm arguing for explicit return values,
not "0 means failure". But I don't care enough to argue, I'm
respinning. Thanks,

Josef

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net v2] net: skbuff: make skb_copy_and_csum_bits() report unreadable frags
  2026-10-14 16:52       ` Josef Bacik
@ 2026-10-14 17:27         ` Josef Bacik
  2026-10-14 18:17           ` Josef Bacik
  0 siblings, 1 reply; 9+ messages in thread
From: Josef Bacik @ 2026-10-14 17:27 UTC (permalink / raw)
  To: David Laight
  Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Simon Horman, Andrew Lunn, David Ahern, Ido Schimmel,
	Trond Myklebust, Anna Schumaker, Chuck Lever, Jeff Layton,
	NeilBrown, Olga Kornievskaia, Dai Ngo, Tom Talpey, Mina Almasry,
	Kaiyuan Zhang, Willem de Bruijn, netdev, linux-kernel, linux-nfs,
	stable

On Wed, Oct 14, 2026 at 12:52 PM Josef Bacik <josef@toxicpanda.com> wrote:
> ...ERR_PTR would be fine too? I'm arguing for explicit return values,
> not "0 means failure". But I don't care enough to argue, I'm
> respinning. Thanks,

I respun it with the sum starting from ~0, and it changes what goes on
the wire, so I was wrong above about ~0 not changing the folded result.

csum_add(~0, 0) stays ~0, and csum_fold(~0) is 0x0000 where
csum_fold(0) is 0xffff.  No other 32 bit value folds to 0xffff, so
there's no non-zero value that can stand in for a zero sum.  ICMPv4
folds the raw sum with no pseudo-header, so an echo request with id 0,
seq 0 and an all-zero payload gets a reply with a checksum of 0x0000
instead of 0xffff, and the sender drops it.  I checked that on
loopback: the reply has 0xffff with v2 and 0x0000 with the ~0 seed.

So 0 can't mean failure here without changing checksums, and v3 will
keep the explicit error return from v2.

Thanks,
Josef

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net v2] net: skbuff: make skb_copy_and_csum_bits() report unreadable frags
  2026-10-14 17:27         ` Josef Bacik
@ 2026-10-14 18:17           ` Josef Bacik
  0 siblings, 0 replies; 9+ messages in thread
From: Josef Bacik @ 2026-10-14 18:17 UTC (permalink / raw)
  To: David Laight
  Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Simon Horman, Andrew Lunn, David Ahern, Ido Schimmel,
	Trond Myklebust, Anna Schumaker, Chuck Lever, Jeff Layton,
	NeilBrown, Olga Kornievskaia, Dai Ngo, Tom Talpey, Mina Almasry,
	Kaiyuan Zhang, Willem de Bruijn, netdev, linux-kernel, linux-nfs,
	stable

On Wed, Oct 14, 2026 at 05:27:18PM +0000, Josef Bacik wrote:
> So 0 can't mean failure here without changing checksums, and v3 will
> keep the explicit error return from v2.

Scratch the v3.  With the code staying the same, all it would add is the
reproduction details from my reply to netdev-bot, and v2 still applies
cleanly to net.  So v2 is the version to take as is.

Thanks,
Josef

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2026-10-14 18:19 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-13 18:02 [PATCH net v2] net: skbuff: make skb_copy_and_csum_bits() report unreadable frags Josef Bacik
2026-10-13 18:05 ` netdev-bot+sinfo
2026-10-14 14:04   ` Josef Bacik
2026-10-14 12:51 ` David Laight
2026-10-14 14:04   ` Josef Bacik
2026-10-14 16:30     ` David Laight
2026-10-14 16:52       ` Josef Bacik
2026-10-14 17:27         ` Josef Bacik
2026-10-14 18:17           ` Josef Bacik

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.