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

[Stanislav Fomichev]

On 10/14, Mina Almasry wrote:
> 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.

Ack, I was a bit confused because I was assuming that these are not
using pp at all, but looks like mlx4 does :-/ And I see you drop
__skb_frag_unref so these have to be converted anyway...
And I guess the same for __skb_frag_unref below.. Let me go through the
rest of the patches and see if anything else catches my eye, thanks!