From: Stanislav Fomichev <sdf.kernel@gmail.com>
To: Mina Almasry <almasrymina@google.com>
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-rdma@vger.kernel.org, bpf@vger.kernel.org,
"Ayush Sawal" <ayush.sawal@chelsio.com>,
"Andrew Lunn" <andrew+netdev@lunn.ch>,
"David S. Miller" <davem@davemloft.net>,
"Eric Dumazet" <edumazet@kernel.org>,
"Jakub Kicinski" <kuba@kernel.org>,
"Paolo Abeni" <pabeni@redhat.com>,
"Tariq Toukan" <tariqt@nvidia.com>,
"Simon Horman" <horms@kernel.org>,
"Steffen Klassert" <steffen.klassert@secunet.com>,
"Herbert Xu" <herbert@gondor.apana.org.au>,
"Neal Cardwell" <ncardwell@google.com>,
"Kuniyuki Iwashima" <kuniyu@google.com>,
"John Fastabend" <john.fastabend@gmail.com>,
"Sabrina Dubroca" <sd@queasysnail.net>,
"Eric Biggers" <ebiggers@kernel.org>,
"Kees Cook" <kees@kernel.org>,
"Aleksandr Loktionov" <aleksandr.loktionov@intel.com>,
"Petr Machata" <petrm@nvidia.com>,
"Uwe Kleine-König (The Capable Hub)"
<u.kleine-koenig@baylibre.com>,
"Jakub Raczynski" <j.raczynski@samsung.com>,
"Wang Zhan" <wang.zhan@smartx.com>,
"Willem de Bruijn" <willemb@google.com>
Subject: Re: [PATCH net-next v3 3/6] net: skbuff: replace skb_page_unref() and __skb_frag_unref() with skb_netmem_unref()
Date: Wed, 14 Oct 2026 14:25:21 -0700 [thread overview]
Message-ID: <as_yg4hCCfpkukZB@devvm7509.cco0.facebook.com> (raw)
In-Reply-To: <20261014173945.373295-4-almasrymina@google.com>
On 10/14, Mina Almasry wrote:
> 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). Having both skb_page_unref() and __skb_frag_unref(frag,
> recycle) made it easy for callers to pass hardcoded recycle=false on
> SKB-owned fragments.
>
> Replace skb_page_unref() and __skb_frag_unref() with
> skb_netmem_unref(netmem, recycle), which dispatches to
> napi_pp_put_page() when recycle is true and put_netmem() otherwise:
>
> - Use skb_frag_unref(skb, ...) on SKB-owned fragments in mlx4, sky2,
> tls_strp, and skb_shift() so unrefs always match skb->pp_recycle.
> - Use put_netmem() directly in tls_device's destroy_record(), which
> releases raw non-pp page references outside an SKB.
>
> Found by code inspection and confirmed with the KUnit tests added later
> in this series; not observed in production. The mlx4, sky2, and TLS
> call-site changes are compile-tested only.
>
> Assisted-by: LLM
> Signed-off-by: Mina Almasry <almasrymina@google.com>
> ---
> v2:
> - Note how the issue was found and how the change was tested.
> - Link to v1: https://lore.kernel.org/netdev/20261010083935.3274178-4-almasrymina@google.com/
> ---
> drivers/net/ethernet/marvell/sky2.c | 2 +-
> drivers/net/ethernet/mellanox/mlx4/en_rx.c | 2 +-
> include/linux/skbuff_ref.h | 34 +++++++++++-----------
> net/core/skbuff.c | 7 +++--
> net/ipv4/esp4.c | 4 +--
> net/ipv6/esp6.c | 4 +--
> net/tls/tls_device.c | 2 +-
> net/tls/tls_strp.c | 2 +-
> 8 files changed, 29 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/net/ethernet/marvell/sky2.c b/drivers/net/ethernet/marvell/sky2.c
> index 30facdece3e29..1d42a82db8f2b 100644
[..]
> --- a/drivers/net/ethernet/marvell/sky2.c
> +++ b/drivers/net/ethernet/marvell/sky2.c
> @@ -2501,7 +2501,7 @@ static void skb_put_frags(struct sk_buff *skb, unsigned int hdr_space,
>
> if (length == 0) {
> /* don't need this page */
> - __skb_frag_unref(frag, false);
> + skb_frag_unref(skb, i);
> --skb_shinfo(skb)->nr_frags;
> } else {
> size = min(length, (unsigned) PAGE_SIZE);
> 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;
> }
These do not support qops and don't advertise devmem tx, so they are not
going to deal with netmems anytime soon, why touch these?
> 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().
> + */
> +static __always_inline void skb_netmem_unref(netmem_ref netmem, bool recycle)
> {
> #ifdef CONFIG_PAGE_POOL
> if (recycle && napi_pp_put_page(netmem))
> @@ -46,31 +55,22 @@ static __always_inline void skb_page_unref(netmem_ref netmem, bool recycle)
> }
>
> /**
> - * __skb_frag_unref - release a reference on a paged fragment.
> - * @frag: the paged fragment
> - * @recycle: recycle the page if allocated via page_pool
> - *
> - * Releases a reference on the paged fragment @frag
> - * or recycles the page via the page_pool API.
> - */
> -static __always_inline void __skb_frag_unref(skb_frag_t *frag, bool recycle)
> -{
> - skb_page_unref(skb_frag_netmem(frag), recycle);
> -}
> -
> -/**
> - * 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().
> */
> static __always_inline void skb_frag_unref(struct sk_buff *skb, int f)
> {
> struct skb_shared_info *shinfo = skb_shinfo(skb);
>
> if (!skb_zcopy_managed(skb))
> - __skb_frag_unref(&shinfo->frags[f], skb->pp_recycle);
> + skb_netmem_unref(skb_frag_netmem(&shinfo->frags[f]),
> + skb->pp_recycle);
> }
>
> #endif /* _LINUX_SKBUFF_REF_H */
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 76a42c60508d9..980bbe9166393 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -1144,7 +1144,8 @@ static void skb_release_data(struct sk_buff *skb, enum skb_drop_reason reason)
> }
>
> for (i = 0; i < shinfo->nr_frags; i++)
> - __skb_frag_unref(&shinfo->frags[i], skb->pp_recycle);
> + skb_netmem_unref(skb_frag_netmem(&shinfo->frags[i]),
> + skb->pp_recycle);
>
> free_head:
> if (shinfo->frag_list)
> @@ -1154,7 +1155,7 @@ static void skb_release_data(struct sk_buff *skb, enum skb_drop_reason reason)
> exit:
> /* When we clone an SKB we copy the reycling bit. The pp_recycle
> * bit is only set on the head though, so in order to avoid races
> - * while trying to recycle fragments on __skb_frag_unref() we need
> + * while trying to recycle fragments on skb_netmem_unref() we need
> * to make one SKB responsible for triggering the recycle path.
> * So disable the recycling bit if an SKB is cloned and we have
> * additional references to the fragmented part of the SKB.
> @@ -4412,7 +4413,7 @@ int skb_shift(struct sk_buff *tgt, struct sk_buff *skb, int shiftlen)
> fragto = &skb_shinfo(tgt)->frags[merge];
>
> skb_frag_size_add(fragto, skb_frag_size(fragfrom));
> - __skb_frag_unref(fragfrom, skb->pp_recycle);
> + skb_frag_unref(skb, 0);
> }
[..]
> /* Reposition in the original skb */
> diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c
> index e76db5817e78e..ae701561d9f12 100644
> --- a/net/ipv4/esp4.c
> +++ b/net/ipv4/esp4.c
> @@ -117,8 +117,8 @@ static void esp_ssg_unref(struct xfrm_state *x, void *tmp, struct sk_buff *skb,
> struct scatterlist *src = already_unref ? esp_req_sg(aead, req) : req->src;
>
> for (sg = sg_next(src); sg; sg = sg_next(sg))
> - skb_page_unref(page_to_netmem(sg_page(sg)),
> - skb->pp_recycle);
> + skb_netmem_unref(page_to_netmem(sg_page(sg)),
> + skb->pp_recycle);
> }
> }
>
> diff --git a/net/ipv6/esp6.c b/net/ipv6/esp6.c
> index b1c9b36f76dc4..d654f0f442388 100644
> --- a/net/ipv6/esp6.c
> +++ b/net/ipv6/esp6.c
> @@ -134,8 +134,8 @@ static void esp_ssg_unref(struct xfrm_state *x, void *tmp, struct sk_buff *skb,
> struct scatterlist *src = already_unref ? esp_req_sg(aead, req) : req->src;
>
> for (sg = sg_next(src); sg; sg = sg_next(sg))
> - skb_page_unref(page_to_netmem(sg_page(sg)),
> - skb->pp_recycle);
> + skb_netmem_unref(page_to_netmem(sg_page(sg)),
> + skb->pp_recycle);
> }
> }
>
> diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
> index f11d0528fc431..b333a9a72cdae 100644
> --- a/net/tls/tls_device.c
> +++ b/net/tls/tls_device.c
> @@ -143,7 +143,7 @@ static void destroy_record(struct tls_record_info *record)
> int i;
>
> for (i = 0; i < record->num_frags; i++)
> - __skb_frag_unref(&record->frags[i], false);
> + put_netmem(skb_frag_netmem(&record->frags[i]));
> kfree(record);
> }
>
> diff --git a/net/tls/tls_strp.c b/net/tls/tls_strp.c
> index 6cc222008d95c..53d3256d0114a 100644
> --- a/net/tls/tls_strp.c
> +++ b/net/tls/tls_strp.c
> @@ -197,7 +197,7 @@ static void tls_strp_flush_anchor_copy(struct tls_strparser *strp)
> DEBUG_NET_WARN_ON_ONCE(atomic_read(&shinfo->dataref) != 1);
>
> for (i = 0; i < shinfo->nr_frags; i++)
> - __skb_frag_unref(&shinfo->frags[i], false);
> + skb_frag_unref(strp->anchor, i);
> shinfo->nr_frags = 0;
> if (strp->copy_mode) {
> kfree_skb_list(shinfo->frag_list);
Idk this part, seems harmless.. But are you sure we can even reach xfrm
(supposedly that's how we get to esp4) with netmems?
next prev parent reply other threads:[~2026-10-14 21:25 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-10-14 17:37 [PATCH net-next v3 0/6] net: unify symmetric netmem and page_pool refcounting Mina Almasry
2026-10-14 17:37 ` [PATCH net-next v3 1/6] netmem: rename __get/__put_netmem() to get/put_net_iov() Mina Almasry
2026-10-14 17:37 ` [PATCH net-next v3 2/6] net: skbuff: add napi_pp_get_page() and use it in tcp_recvmsg_dmabuf() Mina Almasry
2026-10-14 17:37 ` [PATCH net-next v3 3/6] net: skbuff: replace skb_page_unref() and __skb_frag_unref() with skb_netmem_unref() Mina Almasry
2026-10-14 21:25 ` Stanislav Fomichev [this message]
2026-10-14 22:05 ` Mina Almasry
2026-10-14 22:39 ` Stanislav Fomichev
2026-10-14 17:37 ` [PATCH net-next v3 4/6] net: skbuff: replace __skb_frag_ref() with symmetric skb_netmem_ref() Mina Almasry
2026-10-14 17:37 ` [PATCH net-next v3 5/6] net: skbuff: use skb_frag_ref() in skb_try_coalesce() Mina Almasry
2026-10-14 17:37 ` [PATCH net-next v3 6/6] net: kunit: test netmem, page_pool, and skb frag refcounting Mina Almasry
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=as_yg4hCCfpkukZB@devvm7509.cco0.facebook.com \
--to=sdf.kernel@gmail.com \
--cc=aleksandr.loktionov@intel.com \
--cc=almasrymina@google.com \
--cc=andrew+netdev@lunn.ch \
--cc=ayush.sawal@chelsio.com \
--cc=bpf@vger.kernel.org \
--cc=davem@davemloft.net \
--cc=ebiggers@kernel.org \
--cc=edumazet@kernel.org \
--cc=herbert@gondor.apana.org.au \
--cc=horms@kernel.org \
--cc=j.raczynski@samsung.com \
--cc=john.fastabend@gmail.com \
--cc=kees@kernel.org \
--cc=kuba@kernel.org \
--cc=kuniyu@google.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-rdma@vger.kernel.org \
--cc=ncardwell@google.com \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=petrm@nvidia.com \
--cc=sd@queasysnail.net \
--cc=steffen.klassert@secunet.com \
--cc=tariqt@nvidia.com \
--cc=u.kleine-koenig@baylibre.com \
--cc=wang.zhan@smartx.com \
--cc=willemb@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox