[PATCH net-next 00/15] net: skbuff: follow-ups to the BUG_ON() removal

[PATCH net-next 00/15] net: skbuff: follow-ups to the BUG_ON() removal

[Josef Bacik]

Hello,

Jakub applied v2 of my skbuff BUG_ON() series[1] before I got v3 out, so
this is what v3 would have changed, as fixes on top of what landed, plus
the skb_copy_and_csum_bits() patch I'd been sending to net on its own.

Patch 1 is that patch[2], moved to net-next as Mina suggested, with his
Reviewed-by.  It makes skb_copy_and_csum_bits() report failure, and the
rest of the series builds on it.  Please drop the net version.

Patches 2 through 6 deal with skb_checksum() and skb_crc32c().  My v2
had them return 0 when they run out of skb, and that turned out not to
be safe: 0 is a valid checksum, and it can make a packet that was never
fully checksummed pass verification.  So they get versions that report
failure, skb_checksum_full() and skb_crc32c_full(), and the core users
switch to them: checksum validation now rejects a packet it can't
checksum, skb_segment() and pskb_trim_rcsum() fail instead of using a
bad sum, and skb_crc32c_csum_help() drops the packet.  skb_checksum()
and skb_crc32c() stay as wrappers that behave as before, and I'll
convert the rest of their callers per subsystem in later series.

Patches 7 through 10 fix problems Sashiko and an LLM review found in
what was applied.  skb_zerocopy() refuses a zero-length copy that Open
vSwitch can trigger from a user namespace, skb_try_coalesce() and
skb_copy_and_csum_bits() can be handed a negative length, and
skb_headlen() can underflow in skb_copy_and_csum_dev().

Patches 11 and 12 make skb_segment() refuse a nested frag_list, which
could still oops on a NULL list_skb, and add KUnit cases for the layouts
it refuses.  Patches 13 and 14 get rid of the same BUG_ON()s in
xen-netfront's bounce_skb() and tls's __skb_nsg(), and patch 15 updates
comments that still describe the old BUG_ON()s.

I tested this with the net_core KUnit tests, a test module that drives
each of these paths under KASAN and fault injection, an Open vSwitch
reproducer for patch 7, and a stress run over veth with netem.  Thanks,

Josef

[1] https://lore.kernel.org/all/20261007-b4-skbuff-bug-on-v2-0-b9a5f732895b@toxicpanda.com/
[2] https://lore.kernel.org/all/20261013-b4-skb-segment-nosg-unreadable-v2-1-6c28d8ffa51e@toxicpanda.com/

---
Josef Bacik (15):
      net: skbuff: make skb_copy_and_csum_bits() report unreadable frags
      net: skbuff: add skb_checksum_full() that reports failure
      net: skbuff: fail checksum validation when the skb can't be checksummed
      net: skbuff: fail segmentation when a segment can't be checksummed
      net: skbuff: fail pskb_trim_rcsum() when it can't checksum the tail
      net: skbuff: add skb_crc32c_full() that reports failure
      net: skbuff: let skb_zerocopy() copy a short skb without head_frag
      net: skbuff: make skb_try_coalesce()'s len unsigned
      net: skbuff: check csum_start against len in skb_copy_and_csum_dev()
      net: skbuff: reject a negative length in skb_copy_and_csum_bits()
      net: skbuff: refuse to segment a nested frag_list
      net: test the frag_list layouts skb_segment() refuses
      xen-netfront: don't BUG() when skb_copy_bits() fails in bounce_skb()
      net/tls: don't BUG() on leftover length in __skb_nsg()
      net: update comments that still describe the skb BUG_ON()s

 drivers/net/ethernet/sun/sunvnet_common.c |  10 +-
 drivers/net/xen-netfront.c                |   7 +-
 include/linux/skbuff.h                    |   8 +-
 net/core/dev.c                            |   7 +-
 net/core/filter.c                         |   6 +-
 net/core/net_test.c                       |  52 +++++-
 net/core/skbuff.c                         | 274 +++++++++++++++++++++++-------
 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 +-
 net/tls/tls_sw.c                          |   5 +-
 13 files changed, 327 insertions(+), 89 deletions(-)
---
base-commit: 6b3e73774815910a39d8ca75b9ad7df963f61d50
change-id: 20261016-b4-skbuff-bug-on-followups-ad11adf285a4

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

[Josef Bacik]

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)) {
+				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 af1cbc2799b7..09db32f70b53 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -4277,8 +4277,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 93c2f440b9ec..a2b52a5a61a9 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3630,14 +3630,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 or runs
+ *	out before @len does.  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) {
@@ -3646,7 +3659,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;
@@ -3656,7 +3669,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++) {
@@ -3688,7 +3701,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;
 		}
@@ -3705,12 +3718,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;
@@ -3722,9 +3739,11 @@ __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;
 	}
-	return csum;
+out:
+	*csump = csum;
+	return 0;
 }
 EXPORT_SYMBOL(skb_copy_and_csum_bits);
 
@@ -4006,9 +4025,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;
@@ -5057,11 +5080,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 3396f177a18d..f7da4538d05b 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 25fe0ba98b1a..d259e7dca8ba 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))

-- 
2.55.0

[PATCH net-next 02/15] net: skbuff: add skb_checksum_full() that reports failure

[Josef Bacik]

skb_checksum() can't fail.  When the range runs into unreadable frags
or past the end of the skb it returns 0, which is also a valid
checksum, so its callers can't tell the sum from a failure.  For the
receive-side validation helpers that means a packet whose payload was
never checksummed can pass, and commit 20ddd0fd2fcd ("net: skbuff:
don't BUG() on leftover length in skb_checksum() and friends") made the
past-the-end case one of those instead of a BUG().

Add skb_checksum_full(), which adds the range's sum to *@csump and
returns 0, or returns -EFAULT for unreadable frags or a range past the
end of the skb, and -EINVAL for a negative length, leaving *@csump
alone.  It's __must_check so a caller can't ignore the failure, and it
recurses into frag_list members through itself so a failure there
isn't lost either.

skb_checksum() becomes a wrapper around it that behaves as before for
the callers that haven't been converted: it returns 0 on failure and
warns once if that was because of unreadable frags.  The following
patches convert the core users, and the rest will follow per subsystem.

Assisted-by: LLM
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 include/linux/skbuff.h |  2 ++
 net/core/skbuff.c      | 56 +++++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 48 insertions(+), 10 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 09db32f70b53..969b1c3269ec 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -4279,6 +4279,8 @@ 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);
 int __must_check skb_copy_and_csum_bits(const struct sk_buff *skb, int offset,
 					u8 *to, int len, __wsum *csum);
+int __must_check skb_checksum_full(const struct sk_buff *skb, int offset,
+				   int len, __wsum *csump);
 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 a2b52a5a61a9..061c61422a38 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3545,12 +3545,31 @@ int skb_store_bits(struct sk_buff *skb, int offset, const void *from, int len)
 EXPORT_SYMBOL(skb_store_bits);
 
 /* Checksum skb data. */
-__wsum skb_checksum(const struct sk_buff *skb, int offset, int len, __wsum csum)
+/**
+ *	skb_checksum_full - checksum a range of an skb
+ *	@skb: buffer to checksum
+ *	@offset: offset in @skb to start at
+ *	@len: number of bytes to checksum
+ *	@csump: the sum to add to; on success the range's sum is added to it
+ *
+ *	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 *@csump is not changed.
+ */
+int skb_checksum_full(const struct sk_buff *skb, int offset, int len,
+		      __wsum *csump)
 {
 	int start = skb_headlen(skb);
 	int i, copy = start - offset;
 	struct sk_buff *frag_iter;
+	__wsum csum = *csump;
 	int pos = 0;
+	int err;
+
+	if (unlikely(len < 0)) {
+		DEBUG_NET_WARN_ON_ONCE(1);
+		return -EINVAL;
+	}
 
 	/* Checksum header. */
 	if (copy > 0) {
@@ -3558,13 +3577,13 @@ __wsum skb_checksum(const struct sk_buff *skb, int offset, int len, __wsum csum)
 			copy = len;
 		csum = csum_partial(skb->data + offset, copy, csum);
 		if ((len -= copy) == 0)
-			return csum;
+			goto out;
 		offset += copy;
 		pos	= copy;
 	}
 
-	if (WARN_ON_ONCE(!skb_frags_readable(skb)))
-		return 0;
+	if (!skb_frags_readable(skb))
+		return -EFAULT;
 
 	for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
 		int end;
@@ -3593,7 +3612,7 @@ __wsum skb_checksum(const struct sk_buff *skb, int offset, int len, __wsum csum)
 			}
 
 			if (!(len -= copy))
-				return csum;
+				goto out;
 			offset += copy;
 		}
 		start = end;
@@ -3606,14 +3625,17 @@ __wsum skb_checksum(const struct sk_buff *skb, int offset, int len, __wsum csum)
 
 		end = start + frag_iter->len;
 		if ((copy = end - offset) > 0) {
-			__wsum csum2;
+			__wsum csum2 = 0;
+
 			if (copy > len)
 				copy = len;
-			csum2 = skb_checksum(frag_iter, offset - start, copy,
-					     0);
+			err = skb_checksum_full(frag_iter, offset - start, copy,
+						&csum2);
+			if (err)
+				return err;
 			csum = csum_block_add(csum, csum2, pos);
 			if ((len -= copy) == 0)
-				return csum;
+				goto out;
 			offset += copy;
 			pos    += copy;
 		}
@@ -3621,9 +3643,23 @@ __wsum skb_checksum(const struct sk_buff *skb, int offset, int len, __wsum csum)
 	}
 	if (unlikely(len)) {
 		DEBUG_NET_WARN_ON_ONCE(1);
-		return 0;
+		return -EFAULT;
 	}
+out:
+	*csump = csum;
+	return 0;
+}
+EXPORT_SYMBOL(skb_checksum_full);
 
+/* skb_checksum() has no way to report failure, so it returns 0, which is a
+ * valid checksum.  Use skb_checksum_full() in new code.
+ */
+__wsum skb_checksum(const struct sk_buff *skb, int offset, int len, __wsum csum)
+{
+	if (unlikely(skb_checksum_full(skb, offset, len, &csum))) {
+		WARN_ON_ONCE(!skb_frags_readable(skb));
+		return 0;
+	}
 	return csum;
 }
 EXPORT_SYMBOL(skb_checksum);

-- 
2.55.0

[PATCH net-next 03/15] net: skbuff: fail checksum validation when the skb can't be checksummed

[Josef Bacik]

__skb_checksum_complete() verifies a received packet with
skb_checksum(), which returns 0 when the range runs into unreadable
frags or past the end of the skb.  0 is a valid sum, so it's treated as
one: it adds skb->csum, the pseudo-header sum, and folds the result.
With a sum of 0 that's csum_fold(skb->csum), which is 0 whenever the
pseudo-header sum folds to 0xffff, so a packet whose payload was never
checksummed is accepted, marked csum_valid, and left with a wrong
CHECKSUM_COMPLETE value in skb->csum.

__skb_checksum_complete_head() passes skb->csum in as the seed, so a
failure there folds 0 and the packet fails validation, but only because
skb_checksum() happened to drop the seed.

Use skb_checksum_full() in both, and report a packet that can't be
checksummed as having a bad checksum, so callers drop it the way they
drop any other bad checksum.  Clear csum_valid, and leave skb->csum and
ip_summed alone, since there's no full packet checksum to store.

Found by the Sashiko AI review of the series that made skb_checksum()
return 0 for a range past the end of the skb.

Fixes: 20ddd0fd2fcd ("net: skbuff: don't BUG() on leftover length in skb_checksum() and friends")
Link: https://lore.kernel.org/all/20261007-b4-skbuff-bug-on-v2-0-b9a5f732895b@toxicpanda.com/
Assisted-by: LLM
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 net/core/skbuff.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 061c61422a38..40d2dd20d47d 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3860,9 +3860,17 @@ EXPORT_SYMBOL(skb_crc32c);
 
 __sum16 __skb_checksum_complete_head(struct sk_buff *skb, int len)
 {
+	__wsum csum = skb->csum;
 	__sum16 sum;
 
-	sum = csum_fold(skb_checksum(skb, 0, len, skb->csum));
+	/* If the data can't be checksummed it can't be valid either. */
+	if (skb_checksum_full(skb, 0, len, &csum)) {
+		if (!skb_shared(skb))
+			skb->csum_valid = 0;
+		return (__force __sum16)1;
+	}
+
+	sum = csum_fold(csum);
 	/* See comments in __skb_checksum_complete(). */
 	if (likely(!sum)) {
 		if (unlikely(skb->ip_summed == CHECKSUM_COMPLETE) &&
@@ -3886,10 +3894,17 @@ EXPORT_SYMBOL(__skb_checksum_complete_head);
  */
 __sum16 __skb_checksum_complete(struct sk_buff *skb)
 {
-	__wsum csum;
+	__wsum csum = 0;
 	__sum16 sum;
 
-	csum = skb_checksum(skb, 0, skb->len, 0);
+	/* If the data can't be checksummed it can't be valid either.  Leave
+	 * skb->csum alone, it's not a full packet checksum.
+	 */
+	if (skb_checksum_full(skb, 0, skb->len, &csum)) {
+		if (!skb_shared(skb))
+			skb->csum_valid = 0;
+		return (__force __sum16)1;
+	}
 
 	sum = csum_fold(csum_add(skb->csum, csum));
 	/* This check is inverted, because we already knew the hardware

-- 
2.55.0

[PATCH net-next 04/15] net: skbuff: fail segmentation when a segment can't be checksummed

[Josef Bacik]

When the device can't checksum the protocol, skb_segment() computes
each segment's checksum in software with skb_checksum().  With SG on,
the segments share the original frags, and a devmem TCP skb keeps its
whole payload in unreadable frags.  The devmem MSG_ZEROCOPY path never
sets SKBFL_SHARED_FRAG, so the __skb_linearize() fallback is skipped
too.  skb_checksum() then warns and returns 0, and tcp_gso_segment()
writes a TCP checksum that doesn't cover the payload that goes out.

That's reachable with tx-checksumming off and SG on: sk_setup_caps()
still gives TCP SG and HW_CSUM, so the dmabuf send is accepted, and
harmonize_features() strips the checksum features but keeps SG.

Use skb_checksum_full() and fail the segmentation with -EFAULT when it
can't checksum a segment, as skb_segment() already does when it can't
copy one.

Found by the Sashiko AI review of an earlier fix for the same path with
SG off.

Link: https://lore.kernel.org/all/20261009-b4-skb-segment-nosg-unreadable-v1-1-7e2334a55fe8@toxicpanda.com/
Assisted-by: LLM
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 net/core/skbuff.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 40d2dd20d47d..88ed2dd99dc0 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -5239,9 +5239,13 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
 
 			if (!nskb->remcsum_offload)
 				nskb->ip_summed = CHECKSUM_NONE;
-			SKB_GSO_CB(nskb)->csum =
-				skb_checksum(nskb, doffset,
-					     nskb->len - doffset, 0);
+			SKB_GSO_CB(nskb)->csum = 0;
+			if (skb_checksum_full(nskb, doffset,
+					      nskb->len - doffset,
+					      &SKB_GSO_CB(nskb)->csum)) {
+				err = -EFAULT;
+				goto err;
+			}
 			SKB_GSO_CB(nskb)->csum_start =
 				skb_headroom(nskb) + doffset;
 		}

-- 
2.55.0

[PATCH net-next 05/15] net: skbuff: fail pskb_trim_rcsum() when it can't checksum the tail

[Josef Bacik]

For a CHECKSUM_COMPLETE skb, pskb_trim_rcsum() subtracts the checksum
of the bytes it trims from skb->csum.  It gets that checksum from
skb_checksum(), so if the trimmed range runs into unreadable frags in a
frag_list member, or past the end of the skb, it subtracts 0 and
leaves skb->csum covering bytes that are no longer in the packet.

pskb_trim_rcsum() can already fail, and its callers drop the packet
when it does.  Use skb_checksum_full() and return its error.

Assisted-by: LLM
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 net/core/skbuff.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 88ed2dd99dc0..de433d452939 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -2829,9 +2829,13 @@ static int pskb_trim_rcsum_complete(struct sk_buff *skb, unsigned int len)
 	int delta = skb->len - len;
 
 	if (skb_frags_readable(skb)) {
-		skb->csum = csum_block_sub(skb->csum,
-					   skb_checksum(skb, len, delta, 0),
-					   len);
+		__wsum csum = 0;
+		int err;
+
+		err = skb_checksum_full(skb, len, delta, &csum);
+		if (err)
+			return err;
+		skb->csum = csum_block_sub(skb->csum, csum, len);
 		return 0;
 	}
 

-- 
2.55.0

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

[Josef Bacik]

skb_crc32c() has the same problem as skb_checksum(): when the range runs
into unreadable frags or past the end of the skb it returns 0, which is
also a valid CRC.  sctp_compute_cksum() complements it, so a failed
walk looks like a checksum of 0xffffffff whatever the packet contains,
and the packet is accepted whenever its checksum field holds that.

Add skb_crc32c_full(), which updates *@crcp and returns 0, or returns
-EFAULT or -EINVAL and leaves *@crcp alone, like skb_checksum_full().
skb_crc32c() becomes a wrapper that behaves as before for the callers
that haven't been converted.

Convert skb_crc32c_csum_help(), which already returns an error when it
can't fill in the CRC, so a transmitted SCTP packet that can't be
checksummed is dropped instead of going out with a CRC of 0xffffffff.
The SCTP and other callers will follow with the skb_checksum() callers.

Fixes: 20ddd0fd2fcd ("net: skbuff: don't BUG() on leftover length in skb_checksum() and friends")
Assisted-by: LLM
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 include/linux/skbuff.h |  2 ++
 net/core/dev.c         |  7 +++++--
 net/core/skbuff.c      | 51 ++++++++++++++++++++++++++++++++++++++++++--------
 3 files changed, 50 insertions(+), 10 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 969b1c3269ec..327a01a5d6d0 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -4329,6 +4329,8 @@ static inline int memcpy_to_msg(struct msghdr *msg, void *data, int len)
 
 __wsum skb_checksum(const struct sk_buff *skb, int offset, int len,
 		    __wsum csum);
+int __must_check skb_crc32c_full(const struct sk_buff *skb, int offset,
+				 int len, u32 *crcp);
 u32 skb_crc32c(const struct sk_buff *skb, int offset, int len, u32 crc);
 
 static inline void * __must_check
diff --git a/net/core/dev.c b/net/core/dev.c
index 8eb9bf1ff9eb..1d34fa4544a2 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3738,8 +3738,11 @@ int skb_crc32c_csum_help(struct sk_buff *skb)
 	if (ret)
 		goto out;
 
-	crc = ~skb_crc32c(skb, start, skb->len - start, ~0);
-	*(__le32 *)(skb->data + offset) = cpu_to_le32(crc);
+	crc = ~0;
+	ret = skb_crc32c_full(skb, start, skb->len - start, &crc);
+	if (ret)
+		goto out;
+	*(__le32 *)(skb->data + offset) = cpu_to_le32(~crc);
 	skb_reset_csum_not_inet(skb);
 out:
 	return ret;
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)
 {
 	int start = skb_headlen(skb);
 	int i, copy = start - offset;
 	struct sk_buff *frag_iter;
+	u32 crc = *crcp;
+	int err;
+
+	if (unlikely(len < 0)) {
+		DEBUG_NET_WARN_ON_ONCE(1);
+		return -EINVAL;
+	}
 
 	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;
 
 	for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
 		int end;
@@ -3829,7 +3847,7 @@ u32 skb_crc32c(const struct sk_buff *skb, int offset, int len, u32 crc)
 			}
 			len -= copy;
 			if (len == 0)
-				return crc;
+				goto out;
 			offset += copy;
 		}
 		start = end;
@@ -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;
 			len -= copy;
 			if (len == 0)
-				return crc;
+				goto out;
 			offset += copy;
 		}
 		start = end;
 	}
 	if (unlikely(len)) {
 		DEBUG_NET_WARN_ON_ONCE(1);
-		return 0;
+		return -EFAULT;
 	}
+out:
+	*crcp = crc;
+	return 0;
+}
+EXPORT_SYMBOL(skb_crc32c_full);
 
+/* 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));
+		return 0;
+	}
 	return crc;
 }
 EXPORT_SYMBOL(skb_crc32c);

-- 
2.55.0

[PATCH net-next 07/15] net: skbuff: let skb_zerocopy() copy a short skb without head_frag

[Josef Bacik]

Commit 433380f5ab8f ("net: skbuff: don't BUG() on a missing head_frag in
skb_zerocopy()") turned the BUG_ON(!from->head_frag && !hlen) into an
-EFAULT return, but left it ahead of the small-payload path.  That path
copies everything with skb_copy_bits() and never references the head
as a page, so the check doesn't apply to it.

Open vSwitch gets there with an ordinary request.
OVS_PACKET_CMD_EXECUTE allocates the packet with __dev_alloc_skb(...,
GFP_KERNEL), which gives it a kmalloc'd head without head_frag.  A
POP_ETH action on a 14 byte frame leaves it empty, and a USERSPACE
action then queues it with queue_userspace_packet(), which calls
skb_zerocopy(user_skb, skb, 0, 0).  That used to BUG(), and now the
upcall fails and the packet is counted as lost, with a WARN on
CONFIG_DEBUG_NET kernels.  OVS_PACKET_CMD_EXECUTE only needs
CAP_NET_ADMIN in the user namespace that owns the network namespace,
so with the openvswitch module loaded any user can do this from their
own namespace.

Move the check below the small-payload path, so a zero-length copy
succeeds the same way it does for an skb with a head_frag, and document
the -EFAULT case.

Found by the Sashiko AI review of v2 of the series that made the
change.

Fixes: 433380f5ab8f ("net: skbuff: don't BUG() on a missing head_frag in skb_zerocopy()")
Link: https://lore.kernel.org/all/179173804997.434549.4450793967361922984@kernel.org/
Assisted-by: LLM
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 net/core/skbuff.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 3b2a3747f009..735dd25213bb 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -4014,8 +4014,9 @@ EXPORT_SYMBOL_GPL(skb_zerocopy_headlen);
  *	Return value:
  *	0: everything is OK
  *	-ENOMEM: couldn't orphan frags of @from due to lack of memory
- *	-EFAULT: skb_copy_bits() found some problem with skb geometry, or readable head
- *      payload would be mixed with unreadable frags.
+ *	-EFAULT: skb_copy_bits() found some problem with skb geometry, @from has
+ *	no head_frag and @hlen is 0 while @len doesn't fit in @to's tailroom,
+ *	or readable head payload would be mixed with unreadable frags.
  */
 int
 skb_zerocopy(struct sk_buff *to, struct sk_buff *from, int len, int hlen)
@@ -4026,15 +4027,18 @@ skb_zerocopy(struct sk_buff *to, struct sk_buff *from, int len, int hlen)
 	struct page *page;
 	unsigned int offset;
 
+	/* dont bother with small payloads */
+	if (len <= skb_tailroom(to))
+		return skb_copy_bits(from, 0, skb_put(to, len), len);
+
+	/* Without a head_frag the head can't be referenced as a page, so the
+	 * caller has to ask for it to be copied.
+	 */
 	if (unlikely(!from->head_frag && !hlen)) {
 		DEBUG_NET_WARN_ON_ONCE(1);
 		return -EFAULT;
 	}
 
-	/* dont bother with small payloads */
-	if (len <= skb_tailroom(to))
-		return skb_copy_bits(from, 0, skb_put(to, len), len);
-
 	if (hlen) {
 		ret = skb_copy_bits(from, 0, skb_put(to, hlen), hlen);
 		if (unlikely(ret))

-- 
2.55.0

[PATCH net-next 08/15] net: skbuff: make skb_try_coalesce()'s len unsigned

[Josef Bacik]

skb_try_coalesce() keeps from->len, which is unsigned, in an int.  A
corrupt from->len above INT_MAX turns negative and gets past the
len <= skb_tailroom(to) check.  Commit 279877d86c82 ("net: skbuff:
don't BUG() when skb_copy_bits() fails in copy helpers") moved the copy
into the tailroom ahead of skb_put(), so that check is now the only
thing left in the way: skb_copy_bits() clamps its copy to the negative
length, and memcpy() takes it as an unsigned int, so it becomes a ~2GB
write past the end of @to.  Before that commit skb_put() ran first,
and its overflow check caught such a length before anything was
copied.

Keep len unsigned, like from->len.  skb_tailroom() is never negative,
so the tailroom check then refuses such a length, and every other use
of len already takes an unsigned value.  The truesize sanity check
compares the int delta against len; spell out its delta < 0 case so a
negative delta still warns now that the comparison is unsigned.

Found by the Sashiko AI review of v2 of the series that made the
change.

Fixes: 279877d86c82 ("net: skbuff: don't BUG() when skb_copy_bits() fails in copy helpers")
Link: https://lore.kernel.org/all/20261007-b4-skbuff-bug-on-v2-0-b9a5f732895b@toxicpanda.com/
Assisted-by: LLM
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 net/core/skbuff.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 735dd25213bb..5c89c463f234 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -6392,7 +6392,8 @@ bool skb_try_coalesce(struct sk_buff *to, struct sk_buff *from,
 		      bool *fragstolen, int *delta_truesize)
 {
 	struct skb_shared_info *to_shinfo, *from_shinfo;
-	int i, delta, len = from->len;
+	unsigned int len = from->len;
+	int i, delta;
 
 	*fragstolen = false;
 
@@ -6463,7 +6464,7 @@ bool skb_try_coalesce(struct sk_buff *to, struct sk_buff *from,
 		delta = from->truesize - SKB_TRUESIZE(skb_end_offset(from));
 	}
 
-	WARN_ON_ONCE(delta < len);
+	WARN_ON_ONCE(delta < 0 || delta < len);
 
 	memcpy(to_shinfo->frags + to_shinfo->nr_frags,
 	       from_shinfo->frags,

-- 
2.55.0

[PATCH net-next 09/15] net: skbuff: check csum_start against len in skb_copy_and_csum_dev()

[Josef Bacik]

Commit 56453199d28f ("net: skbuff: don't BUG() on a bad csum_start in
skb_copy_and_csum_dev()") checks the checksum start against both ends
of the linear area before copying up to it.  For an skb that isn't
CHECKSUM_PARTIAL, though, the checksum start is skb_headlen() itself,
so the check compares it to itself and can't fail.  If data_len is
bigger than len, skb_headlen() underflows to about 4GB, and that
becomes the length skb_copy_from_linear_data() copies into the
driver's bounce buffer.

The old BUG_ON() had the same blind spot.  Also check the checksum
start against skb->len, so such an skb takes the existing fallback,
which copies at most skb->len bytes without filling in the checksum.

Found by LLM review after the series that made the change was applied.

Fixes: 56453199d28f ("net: skbuff: don't BUG() on a bad csum_start in skb_copy_and_csum_dev()")
Assisted-by: LLM
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 net/core/skbuff.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 5c89c463f234..782a6bacd568 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -4103,7 +4103,11 @@ void skb_copy_and_csum_dev(const struct sk_buff *skb, u8 *to)
 	else
 		csstart = skb_headlen(skb);
 
+	/* skb_headlen() underflows if data_len is bigger than len, so check
+	 * csstart against len as well.
+	 */
 	if (unlikely(csstart < 0 || csstart > skb_headlen(skb) ||
+		     csstart > skb->len ||
 		     (skb->ip_summed == CHECKSUM_PARTIAL &&
 		      csstart + skb->csum_offset + sizeof(__sum16) >
 		      skb->len))) {

-- 
2.55.0

[PATCH net-next 10/15] net: skbuff: reject a negative length in skb_copy_and_csum_bits()

[Josef Bacik]

Commit 417261779eab ("net: skbuff: don't BUG() on leftover length in
skb_copy_and_csum_bits()") guards its zero-fill against a negative @len
from a broken caller, but such a @len never gets that far.  The head,
frag and frag_list copies all clamp their length to @len, so a negative
one has already turned into a huge copy, or an endless frag walk, by
then.

Reject a negative @len on entry with -EINVAL, with a
DEBUG_NET_WARN_ON_ONCE() for debug kernels, and drop the guard on the
zero-fill that can no longer be reached with one.  Every caller already
handles an error from skb_copy_and_csum_bits().

Found by the Sashiko AI review of v2 of the series that made the
change.

Fixes: 417261779eab ("net: skbuff: don't BUG() on leftover length in skb_copy_and_csum_bits()")
Link: https://lore.kernel.org/all/20261007-b4-skbuff-bug-on-v2-0-b9a5f732895b@toxicpanda.com/
Assisted-by: LLM
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 net/core/skbuff.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 782a6bacd568..56cec58bccb7 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3678,9 +3678,10 @@ EXPORT_SYMBOL(skb_checksum);
  *	@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 or runs
- *	out before @len does.  On failure *@csump is not set and the part of
- *	@to that couldn't be filled is zeroed.
+ *	Return: 0 on success, -EINVAL if @len is negative, or -EFAULT if @skb
+ *	has unreadable frags or runs out before @len does.  On failure *@csump
+ *	is not set, and on -EFAULT 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)
@@ -3692,6 +3693,14 @@ int skb_copy_and_csum_bits(const struct sk_buff *skb, int offset,
 	__wsum csum = 0;
 	int err;
 
+	/* Everything below clamps copies to @len, so a negative one would
+	 * turn into a huge copy.
+	 */
+	if (unlikely(len < 0)) {
+		DEBUG_NET_WARN_ON_ONCE(1);
+		return -EINVAL;
+	}
+
 	/* Copy header. */
 	if (copy > 0) {
 		if (copy > len)
@@ -3777,8 +3786,7 @@ int skb_copy_and_csum_bits(const struct sk_buff *skb, int offset,
 	if (unlikely(len)) {
 		DEBUG_NET_WARN_ON_ONCE(1);
 		/* Don't hand the caller a buffer with stale bytes in it. */
-		if (len > 0)
-			memset(to, 0, len);
+		memset(to, 0, len);
 		return -EFAULT;
 	}
 out:

-- 
2.55.0

[PATCH net-next 11/15] net: skbuff: refuse to segment a nested frag_list

[Josef Bacik]

skb_segment() walks head_skb's frag_list, but for each member it only
looks at the linear part and frags[], never at the member's own
frag_list.  head_skb->len still counts those bytes, so with a nested
frag_list the walk runs out of members while it still owes data, sets
list_skb to NULL, and dereferences it on the next pass, before any of
the layout checks get a chance to catch it.

Nothing in the tree is known to build such an skb; GRO flushes instead
of chaining an skb that already has a frag_list.  Check for it up front
and fail the segmentation with -EINVAL, with a DEBUG_NET_WARN_ON_ONCE()
for debug kernels, like the other frag_list layout checks.

Assisted-by: LLM
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 net/core/skbuff.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 56cec58bccb7..b6ada54ecc57 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -4962,6 +4962,17 @@ struct sk_buff *skb_segment_list(struct sk_buff *skb,
 }
 EXPORT_SYMBOL_GPL(skb_segment_list);
 
+/* skb_segment() only walks one level of frag_list. */
+static bool skb_segment_nested_frag_list(const struct sk_buff *head_skb)
+{
+	const struct sk_buff *iter;
+
+	skb_walk_frags(head_skb, iter)
+		if (skb_has_frag_list(iter))
+			return true;
+	return false;
+}
+
 /**
  *	skb_segment - Perform protocol segmentation on skb.
  *	@head_skb: buffer to segment
@@ -5023,6 +5034,16 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
 	sg = !!(features & NETIF_F_SG);
 	csum = !!can_checksum_protocol(features, proto);
 
+	/* The walk below never looks inside a member's own frag_list, but
+	 * head_skb->len counts those bytes, so it would run out of members
+	 * and dereference a NULL list_skb.
+	 */
+	if (unlikely(list_skb && skb_segment_nested_frag_list(head_skb))) {
+		DEBUG_NET_WARN_ON_ONCE(1);
+		err = -EINVAL;
+		goto err;
+	}
+
 	if (sg && csum && !gso_by_frags)  {
 		if (!max_segs && !(features & NETIF_F_GSO_PARTIAL)) {
 			struct sk_buff *iter;

-- 
2.55.0

[PATCH net-next 12/15] net: test the frag_list layouts skb_segment() refuses

[Josef Bacik]

The GSO KUnit test only checks layouts skb_segment() can segment.  Add
two it has to refuse with -EINVAL: the non-uniform frag_list with SG
on, which used to hit the BUG_ON(i >= nfrags) the existing test steps
around, and a frag_list member that has its own frag_list.

Both warn on CONFIG_DEBUG_NET kernels, so run them with the warning
backtrace suppressed.

Assisted-by: LLM
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 net/core/net_test.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/net/core/net_test.c b/net/core/net_test.c
index bc2ef7612e7d..12a175268a01 100644
--- a/net/core/net_test.c
+++ b/net/core/net_test.c
@@ -39,6 +39,8 @@ enum gso_test_nr {
 	GSO_TEST_RESEGMENT_MULTI,
 	GSO_TEST_RESEGMENT_TAIL,
 	GSO_TEST_RESEGMENT_ONE_MSS,
+	GSO_TEST_FRAG_LIST_NON_UNIFORM_SG,
+	GSO_TEST_FRAG_LIST_NESTED,
 };
 
 struct gso_test_case {
@@ -57,6 +59,9 @@ struct gso_test_case {
 	unsigned int nr_segs;
 	const unsigned int *segs;
 	const unsigned int *gso_segs;
+
+	/* skb_segment() is expected to fail with this error */
+	int err;
 };
 
 static struct gso_test_case cases[] = {
@@ -204,6 +209,28 @@ static struct gso_test_case cases[] = {
 		},
 		.gso_segs = (const unsigned int[]) { 0, 0, 0, 0 },
 	},
+	{
+		/* frag_list members that don't end on gso_size boundaries
+		 * can't be segmented with SG
+		 */
+		.id = GSO_TEST_FRAG_LIST_NON_UNIFORM_SG,
+		.name = "frag_list_non_uniform_sg",
+		.linear_len = GSO_TEST_SIZE,
+		.nr_frag_skbs = 4,
+		.frag_skbs = (const unsigned int[]) { GSO_TEST_SIZE, 1, GSO_TEST_SIZE, 2 },
+		.err = -EINVAL,
+	},
+	{
+		/* the second frag_list member hangs off the first one's
+		 * frag_list instead of off the head's
+		 */
+		.id = GSO_TEST_FRAG_LIST_NESTED,
+		.name = "frag_list_nested",
+		.linear_len = GSO_TEST_SIZE,
+		.nr_frag_skbs = 2,
+		.frag_skbs = (const unsigned int[]) { GSO_TEST_SIZE, GSO_TEST_SIZE },
+		.err = -EINVAL,
+	},
 };
 
 static void gso_test_case_to_desc(struct gso_test_case *t, char *desc)
@@ -283,6 +310,15 @@ static void gso_test_func(struct kunit *test)
 
 		if (tcase->id == GSO_TEST_GSO_BY_FRAGS)
 			skb_shinfo(skb)->gso_size = GSO_BY_FRAGS;
+
+		if (tcase->id == GSO_TEST_FRAG_LIST_NESTED) {
+			struct sk_buff *first = skb_shinfo(skb)->frag_list;
+
+			skb_shinfo(first)->frag_list = first->next;
+			first->next = NULL;
+			first->len += tcase->frag_skbs[1];
+			first->data_len += tcase->frag_skbs[1];
+		}
 	}
 
 	features = NETIF_F_SG | NETIF_F_HW_CSUM;
@@ -296,6 +332,18 @@ static void gso_test_func(struct kunit *test)
 		features &= ~NETIF_F_SG;
 
 	SKB_GSO_CB(skb)->max_segs = tcase->max_segs;
+
+	if (tcase->err) {
+		/* the suppression block doesn't run if it can't be set up */
+		segs = NULL;
+		kunit_warning_suppress(test)
+			segs = skb_segment(skb, features);
+		KUNIT_EXPECT_EQ(test, PTR_ERR_OR_ZERO(segs), tcase->err);
+		if (!IS_ERR_OR_NULL(segs))
+			kfree_skb_list(segs);
+		goto free_gso_skb;
+	}
+
 	segs = skb_segment(skb, features);
 	if (IS_ERR(segs)) {
 		KUNIT_FAIL(test, "segs error %pe", segs);

-- 
2.55.0

[PATCH net-next 13/15] xen-netfront: don't BUG() when skb_copy_bits() fails in bounce_skb()

[Josef Bacik]

bounce_skb() is an open-coded skb_copy() for skbs whose data isn't
page-aligned, and it has the same BUG_ON(skb_copy_bits()).
skb_copy_bits() only fails when the skb's lengths don't add up.

bounce_skb() already frees the new skb and returns NULL when the
allocation comes back misaligned, and xennet_start_xmit() drops the
packet in that case.  Take the same path, with a
DEBUG_NET_WARN_ON_ONCE() for debug kernels.

Assisted-by: LLM
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 drivers/net/xen-netfront.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index fdcb91042f29..17811d81937c 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -695,7 +695,12 @@ static struct sk_buff *bounce_skb(const struct sk_buff *skb)
 	/* Set the tail pointer and length */
 	skb_put(n, skb->len);
 
-	BUG_ON(skb_copy_bits(skb, -headerlen, n->head, headerlen + skb->len));
+	if (unlikely(skb_copy_bits(skb, -headerlen, n->head,
+				   headerlen + skb->len))) {
+		DEBUG_NET_WARN_ON_ONCE(1);
+		kfree_skb(n);
+		return NULL;
+	}
 
 	skb_copy_header(n, skb);
 	return n;

-- 
2.55.0

[PATCH net-next 14/15] net/tls: don't BUG() on leftover length in __skb_nsg()

[Josef Bacik]

__skb_nsg() counts the scatterlist entries for a range of an skb with a
copy of the __skb_to_sgvec() walk, including its BUG_ON(len) when the
skb runs out before the range does.  tls_decrypt_sg() calls it on the
same range it then passes to skb_to_sgvec(), so a bad range would BUG()
here before it got to skb_to_sgvec()'s error return.

__skb_nsg() already returns -EMSGSIZE for a frag_list nested too deep,
and tls_decrypt_sg() fails the record on any return below 1.  Return
-EINVAL for the leftover length too, with a DEBUG_NET_WARN_ON_ONCE() for
debug kernels.

Assisted-by: LLM
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 net/tls/tls_sw.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 312e51270f29..1c578ea47a1b 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -142,7 +142,10 @@ static int __skb_nsg(struct sk_buff *skb, int offset, int len,
                         start = end;
                 }
         }
-        BUG_ON(len);
+	if (unlikely(len)) {
+		DEBUG_NET_WARN_ON_ONCE(1);
+		return -EINVAL;
+	}
         return elt;
 }
 

-- 
2.55.0

[PATCH net-next 15/15] net: update comments that still describe the skb BUG_ON()s

[Josef Bacik]

The skbuff BUG_ON() conversions left a few comments behind:

 - bpf_skb_net_grow() says skb_segment() BUG()s on a frag_list with
   head_frag set at the downgraded MSS.  It fails the segmentation and
   the packet is dropped, which is still why the skb gets linearized.
 - The GSO KUnit test says the non-uniform frag_list case would hit
   BUG_ON(i >= nfrags) with SG.  That check now fails with -EINVAL.
 - The skb_to_sgvec() kernel-doc doesn't list the -EINVAL that
   __skb_to_sgvec() returns when @offset and @len run past the end of
   the skb.

No functional change.

Assisted-by: LLM
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 net/core/filter.c   | 6 +++---
 net/core/net_test.c | 4 ++--
 net/core/skbuff.c   | 5 +++--
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index 3ee1a093337e..f5bce0dc43fe 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -3696,9 +3696,9 @@ static int bpf_skb_net_grow(struct sk_buff *skb, u32 off, u32 len_diff,
 		shinfo->gso_segs = 0;
 
 		/* Due to header growth, MSS needs to be downgraded.
-		 * There is a BUG_ON() when segmenting the frag_list with
-		 * head_frag true, so linearize the skb after downgrading
-		 * the MSS.
+		 * Segmenting a frag_list with head_frag true at the new MSS
+		 * fails and the packet gets dropped, so linearize the skb
+		 * after downgrading the MSS.
 		 */
 		if (!(flags & BPF_F_ADJ_ROOM_FIXED_GSO)) {
 			skb_decrease_gso_size(shinfo, len_diff);
diff --git a/net/core/net_test.c b/net/core/net_test.c
index 12a175268a01..d38dfeed163b 100644
--- a/net/core/net_test.c
+++ b/net/core/net_test.c
@@ -325,8 +325,8 @@ static void gso_test_func(struct kunit *test)
 	if (tcase->id == GSO_TEST_GSO_PARTIAL)
 		features |= NETIF_F_GSO_PARTIAL;
 
-	/* TODO: this should also work with SG,
-	 * rather than hit BUG_ON(i >= nfrags)
+	/* TODO: this should also work with SG, rather than fail the
+	 * i >= nfrags check in skb_segment()
 	 */
 	if (tcase->id == GSO_TEST_FRAG_LIST_NON_UNIFORM)
 		features &= ~NETIF_F_SG;
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index b6ada54ecc57..4822f96fc1c8 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -5554,8 +5554,9 @@ __skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len,
  *
  *	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.
+ *	the number of scatterlist items used, -EMSGSIZE if the contents
+ *	could not fit, or -EINVAL if @offset and @len run past the end of
+ *	the skb.
  */
 int skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
 {

-- 
2.55.0

Re: [PATCH net-next 15/15] net: update comments that still describe the skb BUG_ON()s

[Mina Almasry]

On Mon, Oct 19, 2026 at 6:51 AM Josef Bacik <josef@toxicpanda.com> wrote:
>
> The skbuff BUG_ON() conversions left a few comments behind:
>
>  - bpf_skb_net_grow() says skb_segment() BUG()s on a frag_list with
>    head_frag set at the downgraded MSS.  It fails the segmentation and
>    the packet is dropped, which is still why the skb gets linearized.
>  - The GSO KUnit test says the non-uniform frag_list case would hit
>    BUG_ON(i >= nfrags) with SG.  That check now fails with -EINVAL.
>  - The skb_to_sgvec() kernel-doc doesn't list the -EINVAL that
>    __skb_to_sgvec() returns when @offset and @len run past the end of
>    the skb.
>
> No functional change.
>
> Assisted-by: LLM
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

I would have preferred this change is squashed with the code change;
if you re-spin for another reason maybe do that.

Acked-by: Mina Almasry <almasrymina@google.com>

Re: [PATCH net-next 14/15] net/tls: don't BUG() on leftover length in __skb_nsg()

[Mina Almasry]

On Mon, Oct 19, 2026 at 6:51 AM Josef Bacik <josef@toxicpanda.com> wrote:
>
> __skb_nsg() counts the scatterlist entries for a range of an skb with a
> copy of the __skb_to_sgvec() walk, including its BUG_ON(len) when the
> skb runs out before the range does.  tls_decrypt_sg() calls it on the
> same range it then passes to skb_to_sgvec(), so a bad range would BUG()
> here before it got to skb_to_sgvec()'s error return.
>
> __skb_nsg() already returns -EMSGSIZE for a frag_list nested too deep,
> and tls_decrypt_sg() fails the record on any return below 1.  Return
> -EINVAL for the leftover length too, with a DEBUG_NET_WARN_ON_ONCE() for
> debug kernels.
>
> Assisted-by: LLM
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  net/tls/tls_sw.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index 312e51270f29..1c578ea47a1b 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c
> @@ -142,7 +142,10 @@ static int __skb_nsg(struct sk_buff *skb, int offset, int len,
>                          start = end;
>                  }
>          }
> -        BUG_ON(len);
> +       if (unlikely(len)) {
> +               DEBUG_NET_WARN_ON_ONCE(1);
> +               return -EINVAL;
> +       }

Ugh, why DEBUG_NET_WARN...? Seems WARN_ON_ONCE() is more appropriate,
after all we're downgrading from a BUG_ON(). Another nice thing is
that it could have been done inline:

if (unlikely(WARN_ON_ONCE(len)))
  return -EINVAL;

But I suppose it doesn't matter much

Acked-by: Mina Almasry <almasrymina@google.com>

-- 
Thanks,
Mina

Re: [PATCH net-next 14/15] net/tls: don't BUG() on leftover length in __skb_nsg()

[Willem de Bruijn]

On Mon, Oct 19, 2026 at 11:13 AM Mina Almasry <almasrymina@google.com> wrote:
>
> On Mon, Oct 19, 2026 at 6:51 AM Josef Bacik <josef@toxicpanda.com> wrote:
> >
> > __skb_nsg() counts the scatterlist entries for a range of an skb with a
> > copy of the __skb_to_sgvec() walk, including its BUG_ON(len) when the
> > skb runs out before the range does.  tls_decrypt_sg() calls it on the
> > same range it then passes to skb_to_sgvec(), so a bad range would BUG()
> > here before it got to skb_to_sgvec()'s error return.
> >
> > __skb_nsg() already returns -EMSGSIZE for a frag_list nested too deep,
> > and tls_decrypt_sg() fails the record on any return below 1.  Return
> > -EINVAL for the leftover length too, with a DEBUG_NET_WARN_ON_ONCE() for
> > debug kernels.
> >
> > Assisted-by: LLM
> > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> > ---
> >  net/tls/tls_sw.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> > index 312e51270f29..1c578ea47a1b 100644
> > --- a/net/tls/tls_sw.c
> > +++ b/net/tls/tls_sw.c
> > @@ -142,7 +142,10 @@ static int __skb_nsg(struct sk_buff *skb, int offset, int len,
> >                          start = end;
> >                  }
> >          }
> > -        BUG_ON(len);
> > +       if (unlikely(len)) {
> > +               DEBUG_NET_WARN_ON_ONCE(1);
> > +               return -EINVAL;
> > +       }
>
> Ugh, why DEBUG_NET_WARN...? Seems WARN_ON_ONCE() is more appropriate,
> after all we're downgrading from a BUG_ON(). Another nice thing is
> that it could have been done inline:

Because that still triggers on systems with panic_on_warn. See also

https://lore.kernel.org/all/willemdebruijn.kernel.140f4eac91ab3@gmail.com/

Re: [PATCH net-next 14/15] net/tls: don't BUG() on leftover length in __skb_nsg()

[Mina Almasry]

On Mon, Oct 19, 2026 at 9:23 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> On Mon, Oct 19, 2026 at 11:13 AM Mina Almasry <almasrymina@google.com> wrote:
> >
> > On Mon, Oct 19, 2026 at 6:51 AM Josef Bacik <josef@toxicpanda.com> wrote:
> > >
> > > __skb_nsg() counts the scatterlist entries for a range of an skb with a
> > > copy of the __skb_to_sgvec() walk, including its BUG_ON(len) when the
> > > skb runs out before the range does.  tls_decrypt_sg() calls it on the
> > > same range it then passes to skb_to_sgvec(), so a bad range would BUG()
> > > here before it got to skb_to_sgvec()'s error return.
> > >
> > > __skb_nsg() already returns -EMSGSIZE for a frag_list nested too deep,
> > > and tls_decrypt_sg() fails the record on any return below 1.  Return
> > > -EINVAL for the leftover length too, with a DEBUG_NET_WARN_ON_ONCE() for
> > > debug kernels.
> > >
> > > Assisted-by: LLM
> > > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> > > ---
> > >  net/tls/tls_sw.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> > > index 312e51270f29..1c578ea47a1b 100644
> > > --- a/net/tls/tls_sw.c
> > > +++ b/net/tls/tls_sw.c
> > > @@ -142,7 +142,10 @@ static int __skb_nsg(struct sk_buff *skb, int offset, int len,
> > >                          start = end;
> > >                  }
> > >          }
> > > -        BUG_ON(len);
> > > +       if (unlikely(len)) {
> > > +               DEBUG_NET_WARN_ON_ONCE(1);
> > > +               return -EINVAL;
> > > +       }
> >
> > Ugh, why DEBUG_NET_WARN...? Seems WARN_ON_ONCE() is more appropriate,
> > after all we're downgrading from a BUG_ON(). Another nice thing is
> > that it could have been done inline:
>
> Because that still triggers on systems with panic_on_warn. See also
>
> https://lore.kernel.org/all/willemdebruijn.kernel.140f4eac91ab3@gmail.com/

Heh, looks like we have the opposite instincts here and I missed the
initial conversation. DEBUG_NET_WARN_ is fine by me too. Thanks!

-- 
Thanks,
Mina

Re: [PATCH net-next 11/15] net: skbuff: refuse to segment a nested frag_list

[Willem de Bruijn]

Josef Bacik wrote:
> skb_segment() walks head_skb's frag_list, but for each member it only
> looks at the linear part and frags[], never at the member's own
> frag_list.  head_skb->len still counts those bytes, so with a nested
> frag_list the walk runs out of members while it still owes data, sets
> list_skb to NULL, and dereferences it on the next pass, before any of
> the layout checks get a chance to catch it.
> 
> Nothing in the tree is known to build such an skb; GRO flushes instead
> of chaining an skb that already has a frag_list.  Check for it up front
> and fail the segmentation with -EINVAL, with a DEBUG_NET_WARN_ON_ONCE()
> for debug kernels, like the other frag_list layout checks.
> 
> Assisted-by: LLM
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  net/core/skbuff.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 56cec58bccb7..b6ada54ecc57 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -4962,6 +4962,17 @@ struct sk_buff *skb_segment_list(struct sk_buff *skb,
>  }
>  EXPORT_SYMBOL_GPL(skb_segment_list);
>  
> +/* skb_segment() only walks one level of frag_list. */
> +static bool skb_segment_nested_frag_list(const struct sk_buff *head_skb)
> +{
> +	const struct sk_buff *iter;
> +
> +	skb_walk_frags(head_skb, iter)
> +		if (skb_has_frag_list(iter))
> +			return true;

We can probably check the same without adding an extra skb_walk_frags.

Perhaps

@@ -5025,6 +5025,12 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
 
                while (pos < offset + len) {
                        if (i >= nfrags) {
+                               if (unlikely(!list_skb || skb_has_frag_list(list_skb))) {
+                                       DEBUG_NET_WARN_ON_ONCE(1);
+                                       err = -EINVAL;
+                                       goto err;
+                               }
+

> +	return false;
> +}
> +
>  /**
>   *	skb_segment - Perform protocol segmentation on skb.
>   *	@head_skb: buffer to segment
> @@ -5023,6 +5034,16 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
>  	sg = !!(features & NETIF_F_SG);
>  	csum = !!can_checksum_protocol(features, proto);
>  
> +	/* The walk below never looks inside a member's own frag_list, but
> +	 * head_skb->len counts those bytes, so it would run out of members
> +	 * and dereference a NULL list_skb.
> +	 */
> +	if (unlikely(list_skb && skb_segment_nested_frag_list(head_skb))) {
> +		DEBUG_NET_WARN_ON_ONCE(1);
> +		err = -EINVAL;
> +		goto err;
> +	}
> +
>  	if (sg && csum && !gso_by_frags)  {
>  		if (!max_segs && !(features & NETIF_F_GSO_PARTIAL)) {
>  			struct sk_buff *iter;
> 
> -- 
> 2.55.0
> 

Re: [PATCH net-next 12/15] net: test the frag_list layouts skb_segment() refuses

[Willem de Bruijn]

Josef Bacik wrote:
> The GSO KUnit test only checks layouts skb_segment() can segment.  Add
> two it has to refuse with -EINVAL: the non-uniform frag_list with SG
> on, which used to hit the BUG_ON(i >= nfrags) the existing test steps
> around, and a frag_list member that has its own frag_list.
> 
> Both warn on CONFIG_DEBUG_NET kernels, so run them with the warning
> backtrace suppressed.
> 
> Assisted-by: LLM
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Willem de Bruijn <willemb@google.com>

Thanks!

> ---
>  net/core/net_test.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> diff --git a/net/core/net_test.c b/net/core/net_test.c
> index bc2ef7612e7d..12a175268a01 100644
> --- a/net/core/net_test.c
> +++ b/net/core/net_test.c
> @@ -39,6 +39,8 @@ enum gso_test_nr {
>  	GSO_TEST_RESEGMENT_MULTI,
>  	GSO_TEST_RESEGMENT_TAIL,
>  	GSO_TEST_RESEGMENT_ONE_MSS,
> +	GSO_TEST_FRAG_LIST_NON_UNIFORM_SG,
> +	GSO_TEST_FRAG_LIST_NESTED,
>  };
>  
>  struct gso_test_case {
> @@ -57,6 +59,9 @@ struct gso_test_case {
>  	unsigned int nr_segs;
>  	const unsigned int *segs;
>  	const unsigned int *gso_segs;
> +
> +	/* skb_segment() is expected to fail with this error */
> +	int err;
>  };
>  
>  static struct gso_test_case cases[] = {
> @@ -204,6 +209,28 @@ static struct gso_test_case cases[] = {
>  		},
>  		.gso_segs = (const unsigned int[]) { 0, 0, 0, 0 },
>  	},
> +	{
> +		/* frag_list members that don't end on gso_size boundaries
> +		 * can't be segmented with SG
> +		 */
> +		.id = GSO_TEST_FRAG_LIST_NON_UNIFORM_SG,
> +		.name = "frag_list_non_uniform_sg",
> +		.linear_len = GSO_TEST_SIZE,
> +		.nr_frag_skbs = 4,
> +		.frag_skbs = (const unsigned int[]) { GSO_TEST_SIZE, 1, GSO_TEST_SIZE, 2 },
> +		.err = -EINVAL,
> +	},
> +	{
> +		/* the second frag_list member hangs off the first one's
> +		 * frag_list instead of off the head's
> +		 */
> +		.id = GSO_TEST_FRAG_LIST_NESTED,
> +		.name = "frag_list_nested",
> +		.linear_len = GSO_TEST_SIZE,
> +		.nr_frag_skbs = 2,
> +		.frag_skbs = (const unsigned int[]) { GSO_TEST_SIZE, GSO_TEST_SIZE },
> +		.err = -EINVAL,
> +	},
>  };
>  
>  static void gso_test_case_to_desc(struct gso_test_case *t, char *desc)
> @@ -283,6 +310,15 @@ static void gso_test_func(struct kunit *test)
>  
>  		if (tcase->id == GSO_TEST_GSO_BY_FRAGS)
>  			skb_shinfo(skb)->gso_size = GSO_BY_FRAGS;
> +
> +		if (tcase->id == GSO_TEST_FRAG_LIST_NESTED) {
> +			struct sk_buff *first = skb_shinfo(skb)->frag_list;
> +
> +			skb_shinfo(first)->frag_list = first->next;
> +			first->next = NULL;
> +			first->len += tcase->frag_skbs[1];
> +			first->data_len += tcase->frag_skbs[1];
> +		}
>  	}
>  
>  	features = NETIF_F_SG | NETIF_F_HW_CSUM;
> @@ -296,6 +332,18 @@ static void gso_test_func(struct kunit *test)
>  		features &= ~NETIF_F_SG;
>  
>  	SKB_GSO_CB(skb)->max_segs = tcase->max_segs;
> +
> +	if (tcase->err) {
> +		/* the suppression block doesn't run if it can't be set up */
> +		segs = NULL;
> +		kunit_warning_suppress(test)
> +			segs = skb_segment(skb, features);

nit: if respinning, add braces


> +		KUNIT_EXPECT_EQ(test, PTR_ERR_OR_ZERO(segs), tcase->err);
> +		if (!IS_ERR_OR_NULL(segs))
> +			kfree_skb_list(segs);
> +		goto free_gso_skb;
> +	}
> +
>  	segs = skb_segment(skb, features);
>  	if (IS_ERR(segs)) {
>  		KUNIT_FAIL(test, "segs error %pe", segs);
> 
> -- 
> 2.55.0
>