Re: [PATCH net-next v3 3/6] net: skbuff: replace skb_page_unref() and __skb_frag_unref() with skb_netmem_unref()

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] This commit fixes a real refcount imbalance in
  mlx4_en_complete_rx_desc()…
- [Low] The commit message describes the mlx4 RX error-path bug as
  "underflowing the page refcount".
- [Low] At this commit, the new kernel-doc in include/linux/skbuff_ref.h
  names ref-side counterparts that do not match the code.

--

net: skbuff: replace skb_page_unref() and __skb_frag_unref() with skb_netmem_unref()

This patch replaces skb_page_unref() and __skb_frag_unref() with a single
skb_netmem_unref(netmem, recycle) helper. SKB-owned fragment unrefs in
mlx4, sky2, tls_strp and skb_shift() now go through skb_frag_unref(), so
they follow skb->pp_recycle. destroy_record() in tls_device now calls
put_netmem() directly.

> Unreferencing a page_pool fragment with recycle=false decrements the
> non-pp backing refcount instead of pp_ref_count, underflowing the page
> refcount when the fragment was owned via page_pool (as on mlx4's RX
> error path).

[Severity: Low]
Is "underflowing the page refcount" the right description of the mlx4
case?

Each mlx4 frag page holds one page reference owned by the pool
(page_count == 1). Per-frag ownership is tracked in pp_ref_count via
page_pool_fragment_page(page, 1) and page_pool_ref_page().

The old rollback in mlx4_en_complete_rx_desc() called put_page(). That
takes page_count from 1 to 0 and frees the page while page_pool still
counts it as in flight.

Without page checks enabled, free_pages_prepare() returns the page to
the buddy allocator. With checks enabled, free_page_is_bad() reports a
page_pool leak and the page is leaked.

When release is false, the page also stays posted in the RX ring:

		if (release) {
			frags->page = NULL;
		} else {
			page_pool_ref_page(page);
		}

In that case the NIC can DMA into the freed page, and the underflow or
double free only comes later, when page_pool releases the page.

When release is true, the page is freed while pp_ref_count and the
inflight accounting still include it. Page_pool shutdown would then
stall, and no underflow happens at all.

Could the commit message describe this as a premature free of a page
that page_pool still considers in flight, rather than a refcount
underflow?

> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
> index 96f97fa0f5489..fe18606c2ab55 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
> @@ -497,7 +497,7 @@ static int mlx4_en_complete_rx_desc(struct mlx4_en_priv *priv,
>  fail:
>  	while (nr > 0) {
>  		nr--;
> -		__skb_frag_unref(skb_shinfo(skb)->frags + nr, false);
> +		skb_frag_unref(skb, nr);
>  	}
>  	return 0;
>  }

[Severity: Low]
This fixes a real refcount imbalance in mlx4_en_complete_rx_desc().
Should it carry a Fixes: tag?

The fail path looks like it was missed by commit 82b023c97f60 ("eth:
mlx4: use the page pool for Rx buffers"). That commit:

- replaced page_ref_inc() with page_pool_ref_page()
- started each page with page_pool_fragment_page(page, 1)
- added skb_mark_for_recycle() in mlx4_en_process_rx_cq(), which runs
  before mlx4_en_complete_rx_desc() is called

The skb_mark_for_recycle() call is here:

	skb = napi_get_frags(&cq->napi);
	if (unlikely(!skb))
		goto next;
	skb_mark_for_recycle(skb);

The rollback loop was left calling __skb_frag_unref(frag, false). So it
dropped what are now pp_ref_count references through put_netmem(). With
skb->pp_recycle set, the new skb_frag_unref() correctly goes through
napi_pp_put_page().

Right now the fix is folded into a tree-wide helper rename that touches
esp4, esp6, tls_device, tls_strp, sky2 and skbuff.c, and the subject only
mentions the helper replacement. Stable trees that contain 82b023c97f60
need only this one-line change.

Would it make sense to send the mlx4 change on its own with
"Fixes: 82b023c97f60" for net, and the rename as a separate net-next
patch?

> diff --git a/include/linux/skbuff_ref.h b/include/linux/skbuff_ref.h
> index 0441c220fdbdf..04114c64dad99 100644
> --- a/include/linux/skbuff_ref.h
> +++ b/include/linux/skbuff_ref.h
> @@ -36,7 +36,16 @@ static __always_inline void skb_frag_ref(struct sk_buff *skb, int f)
>  
>  bool napi_pp_put_page(netmem_ref netmem);
>  
> -static __always_inline void skb_page_unref(netmem_ref netmem, bool recycle)
> +/**
> + * skb_netmem_unref - release a fragment reference on a netmem
> + * @netmem: netmem to unreference
> + * @recycle: whether page_pool recycling is enabled (e.g. skb->pp_recycle)
> + *
> + * Drops pp_ref_count via napi_pp_put_page() if @recycle is true and @netmem
> + * belongs to a page_pool; otherwise drops a non-pp backing reference via
> + * put_netmem(). Counterpart to skb_netmem_ref().
> + */

[ ... ]

> - * skb_frag_unref - release a reference on a paged fragment of an skb.
> + * skb_frag_unref - release a reference on a paged fragment of an skb
>   * @skb: the buffer
>   * @f: the fragment offset
>   *
> - * Releases a reference on the @f'th paged fragment of @skb.
> + * Drops a reference on the @f'th fragment of @skb via skb_netmem_unref()
> + * (pp_ref_count if skb->pp_recycle and page_pool-backed, else non-pp
> + * backing refcount), skipping managed zerocopy fragments. Counterpart to
> + * skb_frag_ref().
>   */

[Severity: Low]
Do these two counterpart references match the code at this commit?

The skb_netmem_unref() kernel-doc points to skb_netmem_ref(), which does
not exist yet at this revision.

The skb_frag_unref() kernel-doc calls skb_frag_ref() its counterpart.
Here, though, skb_frag_ref() still goes through __skb_frag_ref():

static __always_inline void __skb_frag_ref(skb_frag_t *frag)
{
	get_netmem(skb_frag_netmem(frag));
}

That ignores skb->pp_recycle, while skb_frag_unref() now honours it.

The next patch in the series, "net: skbuff: replace __skb_frag_ref()
with symmetric skb_netmem_ref()", fixes both. It adds
skb_netmem_ref(netmem, recycle) and makes skb_frag_ref() pass
skb->pp_recycle.

After that patch, the only remaining difference is that skb_frag_ref()
has no skb_zcopy_managed() check. That difference predates this series,
and the skb_frag_unref() doc already mentions the skip.

Could the counterpart wording move into that later patch, so the
kernel-doc matches the code at every commit?

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