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

[Mina Almasry <almasrymina@xxxxxxxxxx>]

On Mon, Nov 2, 2026 at 6:28 PM Josef Bacik <josef@xxxxxxxxxxxxxx> wrote:
>
> v1: 
> https://lore.kernel.org/all/20261019-b4-skbuff-bug-on-followups-v1-0-41dbef50b9f7@xxxxxxxxxxxxxx/
> v2: 
> https://lore.kernel.org/all/20261021-b4-skbuff-bug-on-followups-v2-0-2aa780a03524@xxxxxxxxxxxxxx/
>
> v2->v3:
> - 1, 2, 6: move the frag_list recursion into a noinline function, so
>   that taking the address of a local there doesn't give the helpers a
>   stack protector (Paolo).  On x86_64 defconfig plus CONFIG_IP_SCTP
>   none of skb_copy_and_csum_bits(), skb_checksum_full(),
>   skb_crc32c_full() or the two wrappers has one now, and
>   __ip_append_data(), __ip6_append_data() and skb_segment() are
>   unchanged.  The callers that pass a local do gain one.  Besides the
>   seven I listed on v2, that's __skb_gro_checksum_complete() and
>   skb_checksum_help(), which are converted in this version, and
>   skb_crc32c_csum_help().
> - 1: put the Fixes: tags back (Fernando, Sashiko).  csump in the
>   prototype, and drop the "Both of above in one bottle" comment
>   (Fernando).  skb_copy_and_csum_dev() zeroes the whole frame when the
>   copy fails, since leaving the checksum alone doesn't guarantee a bad
>   one (Sashiko).
> - 2, 6: the kernel-doc describes the unreadable check as it is, the
>   wrappers don't blame unreadable frags for -EINVAL, and the commit
>   messages say what changes for a failing frag_list member (Fernando,
>   Sashiko).
> - 2: convert skb_checksum_help() too, as 6 does for its crc32c twin, and
>   add Fixes: 20ddd0fd2fcd.
> - 3: convert __skb_gro_checksum_complete() too (Fernando, Sashiko).
>   Take the skb off CHECKSUM_COMPLETE on failure.  Otherwise conntrack,
>   which doesn't drop on a bad checksum, leaves the pseudo-header sum in
>   skb->csum and TCP's validation accepts the packet after all
>   (Sashiko).  Update the comments, and add Fixes: 65249feb6b3d.
> - 4: the commit message was wrong that devmem TX reaches this checksum.
>   Its skbs carry SKBFL_SHARED_FRAG, so the segment fails to linearize
>   first.  The patch is hardening, and the commit message says so now.
> - 5: when the tail can't be checksummed but what's left can be read,
>   set CHECKSUM_NONE instead of failing the trim, which dropped packets
>   that used to be accepted (Sashiko).  Retitled, and add Fixes: tags.
> - 7, 8: add the Fixes: tags for the original commits, and correct the
>   history in 8's commit message (Sashiko).  8 also says what it takes
>   to reach the memcpy().
> - 7: reject a negative @len or @hlen on entry.  The reorder left nothing
>   ahead of the skb_put(), which panics on one.
> - 8: the truesize check refuses the coalesce instead of only warning,
>   and runs before the head is stolen.  With len unsigned and nothing
>   else, an skb with a len above INT_MAX went from refused to coalesced.
> - 9: check data_len > len directly and zero the frame.  The old check
>   missed CHECKSUM_PARTIAL (Sashiko).  Retitled.
> - 10: drop both len > 0 tests, and correct what the commit message says
>   they covered (Sashiko).
> - 9, 10: drop the Fixes: tags.  Both problems are older than the commits
>   the tags named, and neither has a known trigger.
> - 11: Willem, this one changed again after you OK'd the walk up front.
>   A nested frag_list is only one way for the walk to run out of members.
>   A len that counts bytes the skb doesn't hold gets to the same NULL
>   pointer dereference, and the walk up front doesn't catch that.  So the
>   patch now checks for a NULL list_skb in the three places where the
>   walk moves on to it, and that's all it does: no pass up front, which
>   also takes care of the cost you pointed out for BIG TCP, and none of
>   v2's skb_has_frag_list() tests.  You asked on v2 whether the first two
>   checks were needed.  For a nested frag_list they weren't, for this
>   they are.  12 has a case for each, and taking any one of the three
>   checks out brings the oops back.  The path without SG is left alone
>   again.  The commit message says that IP reassembly does nest
>   frag_lists, just not on GSO skbs (Sashiko).  Retitled.
> - 12: add four cases for a len that counts bytes the skb doesn't hold.
>   Retitled.
> - 13: correct what the commit message says about when bounce_skb() runs
>   and when skb_copy_bits() fails.
> - 14: update the comment above skb_nsg() (Sashiko).
> - 15: one more stale comment, in net_test.c.
> - Picked up Fernando's Reviewed-by on 4.  Dropped Mina's on 1 and
>   Fernando's on 2, since the frag_list loop moved in both, and Willem's
>   on 12, since it grew.
> - Shortened the commit messages.
> - Rebased onto current net-next.
> v1->v2:
> - skb_segment(): check each frag_list member as the walk takes it,
>   instead of an extra pass over the frag_list up front (Willem).
> - KUnit test: braces around the suppressed skb_segment() call (Willem),
>   and the comment update for the non-uniform case moved into the same
>   patch (Mina).
> - Picked up Willem's Reviewed-by and Mina's Acked-bys.
>
> --- Original email (v1) ---
>
> 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@xxxxxxxxxxxxxx/
> [2] 
> https://lore.kernel.org/all/20261013-b4-skb-segment-nosg-unreadable-v2-1-6c28d8ffa51e@xxxxxxxxxxxxxx/
>
> ---
> Changes in v3:
> - Link to v2: 
> https://patch.msgid.link/20261021-b4-skbuff-bug-on-followups-v2-0-2aa780a03524@xxxxxxxxxxxxxx
>
> Changes in v2:
> - Link to v1: 
> https://patch.msgid.link/20261019-b4-skbuff-bug-on-followups-v1-0-41dbef50b9f7@xxxxxxxxxxxxxx
>
> ---
> 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: drop hw csum if trim can't checksum 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: reject data_len > len in copy_and_csum_dev
>       net: skbuff: reject a negative length in skb_copy_and_csum_bits()
>       net: skbuff: don't run off frag_list in skb_segment()
>       net: test the skb 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                    |  12 +-
>  net/core/dev.c                            |  14 +-
>  net/core/filter.c                         |   6 +-
>  net/core/gro.c                            |  11 +-
>  net/core/net_test.c                       | 100 ++++++-
>  net/core/skbuff.c                         | 447 
> +++++++++++++++++++++++-------
>  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                          |   8 +-
>  14 files changed, 515 insertions(+), 147 deletions(-)
> ---
> base-commit: 4dd16e31135ea8437a720222df34e8745f385d6d
> change-id: 20261016-b4-skbuff-bug-on-followups-ad11adf285a4
>

Honestly there is nothing super objectionable about the series, but it
seems a bunch of unrelated fixes, capped at 15 because netdev doesn't
let you have more than 15 patches out at once. I wish you broke it up
into less ambitious problems that the human reviewers can follow. But
maybe that's just me.

Like crc checksum failing in 1 series we can follow, and maybe 1
series for a full failable skb_checksum changes in 1 go so we don't
add the new thing before removing the old? Or is that too infeasible?

-- 
Thanks,
Mina