Re: [PATCH net-next v3 4/6] net: skbuff: replace __skb_frag_ref() with symmetric skb_netmem_ref()

[netdev-bot+sashiko@kernel.org]

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

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

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 fixes reachable page_pool refcount imbalances but
  has no Fixes: tag, so the fix will not be picked up for stable.
- [Low] In the new skb_segment() linearize branch, the bytes being
  linearized are counted twice in nskb->truesize: 1) The code does…

--

net: skbuff: replace __skb_frag_ref() with symmetric skb_netmem_ref()

This patch replaces __skb_frag_ref() with skb_netmem_ref(netmem, recycle).
The new helper takes a pp_ref_count reference when recycle is set and the
netmem is page_pool backed, so it mirrors skb_netmem_unref(). The patch
also stops clearing skb->pp_recycle in skb_release_data() and propagates
pp_recycle to the destination skb in the copy, split, shift and segment
paths. It also linearizes skb_segment() output segments when frag_list
skbs have different pp_recycle bits.

Link: https://lore.kernel.org/netdev/xfrm-iptfs-pp_ref_count-underflow-v4-1-912fa72106f0@secunet.com/

> Previously, __skb_frag_ref() and skb_frag_ref() unconditionally
> incremented the non-pp backing refcount via get_netmem(), while
> skb_frag_unref() decremented pp_ref_count whenever skb->pp_recycle was
> set. Ref-ing a page_pool fragment into a pp_recycle=1 SKB (or clearing
> pp_recycle=0 on an uncloned SKB while clones still held pp_recycle=1)
> leaked the backing non-pp refcount and underflowed pp_ref_count across
> IP-TFS, skb_split(), skb_shift(), and cloned unclone paths.

[Severity: Medium]
Should this patch have a Fixes: tag so stable picks it up?

In the base tree, iptfs_skb_add_frags() takes a raw page reference on
each frag it adds. The destination skb only needs to have the same
pp_recycle as the walk, so it can be a pp_recycle=1 skb:

    __skb_frag_ref(tofrag);  ->  get_netmem()  ->  get_page()

Those references are later dropped through napi_pp_put_page(), which
underflows pp_ref_count. The page can then go back to the pool while the
source skb still points at it. That code came from:

Fixes: 5f2b6a909574 ("xfrm: iptfs: add skb-fragment sharing code")

The commit message also covers only one direction of the imbalance.
The reverse case is reachable through skb_split() from batman-adv:

batadv_frag_create()
    skb_fragment allocated with dev_alloc_skb()   /* pp_recycle = 0 */
    skb_split(skb, skb_fragment, skb->len - fragment_size)
        skb_split_inside_header()    /* moves frags, takes no refs */

If the forwarded RX skb has pp_recycle=1, its page_pool frags end up in a
pp_recycle=0 skb. When skb_fragment is freed, put_page() runs on them and
drops a page reference that page_pool owns.

Both directions look like they can cause a use-after-free of the page,
not just a refcount leak. Could the commit message describe the
skb_split() case and its consequence more precisely?

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 980bbe9166393..96088289c234d 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c

[ ... ]

> @@ -5057,6 +5049,16 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
>  
>  		while (pos < offset + len) {
>  			if (i >= nfrags) {
> +				if (skb_shinfo(nskb)->nr_frags &&
> +				    nskb->pp_recycle != list_skb->pp_recycle) {
> +					nskb->data_len = pos - offset - hsize;
> +					nskb->len += nskb->data_len;
> +					nskb->truesize += nskb->data_len;
                                        ^^^^

[Severity: Low]
Are the linearized bytes counted twice in nskb->truesize here?

nskb was allocated with only hsize + doffset + headroom, so
__skb_linearize() will usually need to grow the head:

__skb_linearize()
    __pskb_pull_tail()
        pskb_expand_head(skb, 0, eat + 128, ...)

nskb->sk is NULL during segmentation, so pskb_expand_head() also adds the
head growth to truesize:

	if (!skb->sk || skb->destructor == sock_edemux)
		skb->truesize += size - osize;

If tailroom was already enough, the head was accounted at allocation time,
so the explicit add over-counts in that case too.

hsize is reset after linearizing, so skip_fraglist only adds the remaining
frag bytes. The final truesize then looks inflated by about the
linearized byte count for each straddling segment.

Is the truesize update needed here? Would adjusting data_len and len be
enough to keep the skb consistent for __pskb_pull_tail()?

> +					if (__skb_linearize(nskb))
> +						goto err;
> +					hsize = pos - offset;
> +					nskb_frag = skb_shinfo(nskb)->frags;
> +				}
>  				if (skb_orphan_frags(list_skb, GFP_ATOMIC) ||
>  				    skb_zerocopy_clone(nskb, list_skb,
>  						       GFP_ATOMIC))

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261014173945.373295-1-almasrymina%40google.com