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

[Mina Almasry]

On Wed, Oct 14, 2026 at 2:25 PM Stanislav Fomichev <sdf.kernel@xxxxxxxxx> wrote:
>
> 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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx/
> > ---
> >  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?
>

They're still using page-backed netmems that have 2 refcounts, and
frag ref/unref needs to pick the right one. Before they were
hardcoding false for the pp-recycle bit, so if a skb->pp_recycle==true
here for any reason, this code would hit a ref imbalance. Now after
this series both skb_frag_ref and skb_frag_unref do the right thing by
checking skb->pp_recycle.

This has nothing to do with net-iovs or devmem.

[...]
> > 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?

Same, this is still using page-backed netmems, and hard-coding fales
for the skb->pp_recycle bit. If anchor->pp_recycle==true ever here,
this code would run into a ref imbalance and potentially crash. Now
skb_frag_ref and skb_frag_unref are mirrors of each other and we don't
have to prove beyond a doubt that dropping a non-pp ref is always the
right thing to do here.

-- 
Thanks,
Mina