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

[netdev-bot+sashiko@kernel.org]

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