[PATCH net] net: skbuff: don't segment unreadable skbs without SG

[PATCH net] net: skbuff: don't segment unreadable skbs without SG

[Josef Bacik]

Without NETIF_F_SG, skb_segment() copies each segment's payload out of
head_skb.  With checksum offload it uses skb_copy_bits(), which fails on
unreadable frags, so skb_segment() errors out.  Without checksum offload
it uses skb_copy_and_csum_bits() and keeps the checksum that returns.

Since commit ab9414ed70bd ("net: skbuff: don't leave stale bytes in
skb_copy_and_csum_bits()") that copy zero-fills whatever it can't read
and returns 0, which is the correct checksum for zeroes.  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.
Before that commit the segments carried uninitialized memory with a
checksum that almost never matched, so they leaked but were dropped.

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 then segments it with neither feature.  With
only SG off the copy goes through skb_copy_bits() and is already
refused, and with only checksum offload off the segments keep sharing
the frags.

Refuse to segment unreadable skbs without SG, which is what the checksum
branch already does through skb_copy_bits().  Check the frag_list
members as well, since their payload is copied the same way.

This was found by the Sashiko AI review of that commit.

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/
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
Follow-up to Sashiko's review of ab9414ed70bd:
https://lore.kernel.org/all/179148235783.434549.14322374227477832817@kernel.org/

Tested with a module that hands skb_segment() a TCPv4 GSO skb whose
3000-byte payload sits in an unreadable frag, with SG off.  Without this
patch it returns three segments with all-zero payloads and a checksum
of 0; with it, -EINVAL.  The same skb with checksum offload was already
refused, and a readable skb still segments and copies normally.

Thanks,
Josef
---
 net/core/skbuff.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 41beaf625421..4ba544b5f3ba 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -4782,6 +4782,18 @@ struct sk_buff *skb_segment_list(struct sk_buff *skb,
 }
 EXPORT_SYMBOL_GPL(skb_segment_list);
 
+static bool skb_segment_unreadable(const struct sk_buff *head_skb)
+{
+	const struct sk_buff *iter;
+
+	if (!skb_frags_readable(head_skb))
+		return true;
+	skb_walk_frags(head_skb, iter)
+		if (!skb_frags_readable(iter))
+			return true;
+	return false;
+}
+
 /**
  *	skb_segment - Perform protocol segmentation on skb.
  *	@head_skb: buffer to segment
@@ -4842,6 +4854,14 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
 	sg = !!(features & NETIF_F_SG);
 	csum = !!can_checksum_protocol(features, proto);
 
+	/* Without SG every segment's payload is copied out of head_skb, which
+	 * can't be done for unreadable frags.
+	 */
+	if (!sg && skb_segment_unreadable(head_skb)) {
+		err = -EINVAL;
+		goto err;
+	}
+
 	if (sg && csum && !gso_by_frags)  {
 		if (!(features & NETIF_F_GSO_PARTIAL)) {
 			struct sk_buff *iter;

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

Re: [PATCH net] net: skbuff: don't segment unreadable skbs without SG

[Mina Almasry]

On Fri, Oct 9, 2026 at 6:32 AM Josef Bacik <josef@toxicpanda.com> wrote:
>
> Without NETIF_F_SG, skb_segment() copies each segment's payload out of
> head_skb.  With checksum offload it uses skb_copy_bits(), which fails on
> unreadable frags, so skb_segment() errors out.  Without checksum offload
> it uses skb_copy_and_csum_bits() and keeps the checksum that returns.
>
> Since commit ab9414ed70bd ("net: skbuff: don't leave stale bytes in
> skb_copy_and_csum_bits()") that copy zero-fills whatever it can't read
> and returns 0, which is the correct checksum for zeroes.  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.
> Before that commit the segments carried uninitialized memory with a
> checksum that almost never matched, so they leaked but were dropped.
>
> 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 then segments it with neither feature.  With
> only SG off the copy goes through skb_copy_bits() and is already
> refused, and with only checksum offload off the segments keep sharing
> the frags.
>
> Refuse to segment unreadable skbs without SG, which is what the checksum
> branch already does through skb_copy_bits().  Check the frag_list
> members as well, since their payload is copied the same way.
>
> This was found by the Sashiko AI review of that commit.
>
> 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/
> Cc: stable@vger.kernel.org
> Assisted-by: LLM
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
> Follow-up to Sashiko's review of ab9414ed70bd:
> https://lore.kernel.org/all/179148235783.434549.14322374227477832817@kernel.org/
>
> Tested with a module that hands skb_segment() a TCPv4 GSO skb whose
> 3000-byte payload sits in an unreadable frag, with SG off.  Without this
> patch it returns three segments with all-zero payloads and a checksum
> of 0; with it, -EINVAL.  The same skb with checksum offload was already
> refused, and a readable skb still segments and copies normally.
>
> Thanks,
> Josef
> ---
>  net/core/skbuff.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 41beaf625421..4ba544b5f3ba 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -4782,6 +4782,18 @@ struct sk_buff *skb_segment_list(struct sk_buff *skb,
>  }
>  EXPORT_SYMBOL_GPL(skb_segment_list);
>
> +static bool skb_segment_unreadable(const struct sk_buff *head_skb)
> +{
> +       const struct sk_buff *iter;
> +
> +       if (!skb_frags_readable(head_skb))
> +               return true;
> +       skb_walk_frags(head_skb, iter)
> +               if (!skb_frags_readable(iter))
> +                       return true;
> +       return false;
> +}
> +
>  /**
>   *     skb_segment - Perform protocol segmentation on skb.
>   *     @head_skb: buffer to segment
> @@ -4842,6 +4854,14 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
>         sg = !!(features & NETIF_F_SG);
>         csum = !!can_checksum_protocol(features, proto);
>
> +       /* Without SG every segment's payload is copied out of head_skb, which
> +        * can't be done for unreadable frags.
> +        */
> +       if (!sg && skb_segment_unreadable(head_skb)) {
> +               err = -EINVAL;
> +               goto err;
> +       }
> +

For this one, IMO we cannot have random unreadable checks in the code
because high level functions will eventually do something that
requires readability deep in the call stack. The code deep inside the
call site will change and we (the LLM) will forget to update the
accompanying unreadable check way up the stack.

I would say for this one, if possible make the innermost helper
requiring readability should perform the check, and return an error,
and then update the callers to handle that error generically. You may
end up with a larger, more complicated change but I think it will be a
more sustainable change than this 1-off.

Thanks,
Mina

Re: [PATCH net] net: skbuff: don't segment unreadable skbs without SG

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message says the 'only checksum offload off' case
  is safe because 'the segments keep sharing the frags'.

--

net: skbuff: don't segment unreadable skbs without SG

This patch makes skb_segment() reject unreadable skbs, including their
frag_list members, when NETIF_F_SG is not set. Without SG the payload is
copied out of head_skb. Since commit ab9414ed70bd, a devmem payload copied
that way turns into zeroes that carry a valid checksum.

Link: https://lore.kernel.org/all/179148235783.434549.14322374227477832817@kernel.org/

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 41beaf625421a..4ba544b5f3bab 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c

[ ... ]

> @@ -4842,6 +4854,14 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
>  	sg = !!(features & NETIF_F_SG);
>  	csum = !!can_checksum_protocol(features, proto);
>  
> +	/* Without SG every segment's payload is copied out of head_skb, which
> +	 * can't be done for unreadable frags.
> +	 */
> +	if (!sg && skb_segment_unreadable(head_skb)) {
> +		err = -EINVAL;
> +		goto err;
> +	}
> +

[Severity: Medium]
Does this guard also need to cover the sg && !csum case?

The commit message says:

  "and with only checksum offload off the segments keep sharing
  the frags."

The segments do share the frags. But in that case skb_segment() still
reaches perform_csum_check, and __copy_skb_header() has already copied the
unreadable bit into each nskb:

net/core/skbuff.c:skb_segment() {
    ...
perform_csum_check:
		if (!csum) {
			if (skb_has_shared_frag(nskb) &&
			    __skb_linearize(nskb))
				goto err;

			if (!nskb->remcsum_offload)
				nskb->ip_summed = CHECKSUM_NONE;
			SKB_GSO_CB(nskb)->csum =
				skb_checksum(nskb, doffset,
					     nskb->len - doffset, 0);
    ...
}

The devmem MSG_ZEROCOPY path in tcp_sendmsg_locked() goes through
skb_zerocopy_iter_stream() and never sets SKBFL_SHARED_FRAG. As far as I
can tell, only the MSG_SPLICE_PAGES branch sets it. That means the
__skb_linearize() fallback is skipped and skb_checksum() is called on an
nskb whose payload is entirely in unreadable frags. skb_checksum() then
hits:

	if (WARN_ON_ONCE(!skb_frags_readable(skb)))
		return 0;

This leaves SKB_GSO_CB(nskb)->csum at 0. tcp_gso_segment() then calls
gso_make_checksum() and writes a TCP checksum that does not cover the
dmabuf payload that actually goes out on the wire.

The setup looks reachable in the same way the commit message describes
for the case where both features are off. sk_setup_caps() grants SG and
HW_CSUM, and validate_xmit_unreadable_skb() lets the skb through.
harmonize_features() strips the checksum and GSO features but keeps SG:

	if (skb->ip_summed != CHECKSUM_NONE &&
	    !can_checksum_protocol(features, type)) {
		features &= ~(NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK);
	}

So with tx-checksumming off and SG on, software GSO reaches skb_segment()
with sg=1 and csum=0. The result is a WARN splat, and the peer drops
segments that will never checksum correctly, including on retransmit.

Would checking !sg || !csum here be more appropriate? Since this is
tagged for stable, could the commit message also be updated so it does
not describe the checksum-off-only case as safe?

>  	if (sg && csum && !gso_by_frags)  {
>  		if (!(features & NETIF_F_GSO_PARTIAL)) {
>  			struct sk_buff *iter;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009-b4-skb-segment-nosg-unreadable-v1-1-7e2334a55fe8%40toxicpanda.com