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

[Josef Bacik <josef@xxxxxxxxxxxxxx>]

On Thu, Oct 22, 2026 at 10:21:26AM +0200, Paolo Abeni wrote:
> Recently Eric did some work to remove return arguments from the fast-path,
> as with some config it gives measurable overhead. See commit
> a02a765bd5c2ae7705144829b2911c96c29db6ba.
>
> S/W csum should not be the fastest path, but AFAICS the UDP output path
> could use it when building the aggregate segments.

I checked this on x86_64 defconfig (CONFIG_STACKPROTECTOR_STRONG=y,
gcc 13.3), looking at which functions call __stack_chk_fail before and
after patches 1-6.

The callers on the output path don't change.  __ip_append_data() and
__ip6_append_data() pass &skb->csum and skb_segment() passes
&SKB_GSO_CB(nskb)->csum, so no local has its address taken.  The two
append functions already have a stack protector today, and
skb_segment() doesn't have one before or after.

You're right about the helpers themselves though.
skb_copy_and_csum_bits() and skb_checksum_full() both gain a stack
protector, because of the &csum2 local in the frag_list recursion.
Moving that loop into a noinline function gets rid of it, and only an
skb with a frag_list pays for it:

                                 before  v2   frag_list loop split out
  skb_copy_and_csum_bits           no    yes  no
  skb_checksum / _full             no    yes  no
  __ip_append_data                 yes   yes  yes
  __ip6_append_data                yes   yes  yes
  skb_segment                      no    no   no

  skb_copy_and_csum_bits          774    905  666 bytes

I'll do that in v3.

These gain one and keep it, since they do pass a local:
__skb_checksum_complete(), __skb_checksum_complete_head(),
pskb_trim_rcsum_slow(), skb_copy_and_csum_dev(), icmp_glue_bits(),
icmpv6_getfrag() and xdr_skb_read_bits().  Each of them goes on to
checksum or copy the payload in software.  I haven't measured cycles.

bloat-o-meter for patches 1-6 with the split is +1177 bytes in total,
most of it the new error paths.

> I'm wondering using a 64 bit return value and encoding there both the csum
> and the error coudl make sense.

I'd like to keep the int return and the pointer.  It's __must_check, so
the compiler catches a caller that ignores the failure, and a packed
value needs every caller to know how to unpack it.  0 or ~0 as the error
doesn't work either, it changes the checksum on the wire for data that
sums to zero:

https://lore.kernel.org/all/179199883858343.2.17884063473154560573@xxxxxxxxxxxxxx/

> Also currently net-next is closed for the merge window, and a bunch of
> changes in this series looks more like cleanup than fixes. Cleanup will have
> to wait for the end of the merge window, fixes can land at any time.

There's no rush on any of it, so I'll resend the whole series when
net-next reopens.

Thanks,
Josef