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