Re: [PATCH net] net-sysfs: release the TX queue tracker before allowing reuse

[Antoine Tenart <atenart@kernel.org>]

On Tue, Sep 29, 2026 at 11:27:54AM +0200, Eric Dumazet wrote:
> On Tue, Sep 29, 2026 at 10:13 AM Antoine Tenart <atenart@kernel.org> wrote:
> >
> > On Sun, Sep 27, 2026 at 01:33:15AM +0800, Chengfeng Ye wrote:
> > > An interrupted sysfs_rtnl_lock() can drop the last kobject reference to a
> > > removed TX queue without holding RTNL. netdev_queue_release() clears the
> > > kobject before releasing queue->dev_tracker, allowing the queue to be
> > > re-added while the old release still needs the shared tracker slot:
> > >
> > >   CPU 0                               CPU 1
> > >   netdev_queue_release()
> > >     memset(kobj, 0, sizeof(*kobj))
> > >                                       netdev_queue_add_kobject()
> > >                                         state_initialized is clear
> > >                                         netdev_hold() installs new tracker
> > >     netdev_put() releases the new tracker
> > >
> > > With CONFIG_NET_DEV_REFCNT_TRACKER enabled, the old tracker is leaked and
> > > the new lifetime's tracker is released prematurely. A later queue release
> > > then reports a double release. The numeric device references remain
> > > balanced.
> > >
> > > The kernel reported:
> > >
> > >   ref_tracker: reference already released.
> > >   ref_tracker: allocated in:
> > >    netdev_queue_update_kobjects+0x23d/0x5c0
> > >    netif_set_real_num_tx_queues+0x111/0x820
> > >    veth_set_channels+0x327/0x930
> > >    ethtool_set_channels+0x3ee/0x490
> > >   ref_tracker: freed in:
> > >    netdev_queue_release+0xbd/0x130
> > >    kobject_put+0x1f9/0x280
> > >    sysfs_rtnl_lock+0x18b/0x1f0
> > >    xps_rxqs_show+0xad/0x250
> > >   WARNING: lib/ref_tracker.c:322 at ref_tracker_free+0x49e/0x6d0
> > >   Call Trace:
> > >    netdev_queue_release+0xbd/0x130
> > >    kobject_put+0x1f9/0x280
> > >    netdev_queue_update_kobjects+0x3f9/0x5c0
> > >    netif_set_real_num_tx_queues+0x111/0x820
> > >    veth_set_channels+0x327/0x930
> > >    ethtool_set_channels+0x3ee/0x490
> > >
> > > Release the tracker before clearing the kobject. Use a full memory barrier
> > > to order the tracker access before clearing state_initialized, paired with
> > > the control dependency from that check to the new tracker allocation.
> > > Keep the device reference until after the reset so that the queue storage
> > > remains alive throughout the callback's accesses.
> > >
> > > Fixes: b0b6fcfa6ad8 ("net-sysfs: remove rtnl_trylock from queue attributes")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
> > > ---
> > >  net/core/net-sysfs.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
> > > index 352173df7578..b9aadc4c000c 100644
> > > --- a/net/core/net-sysfs.c
> > > +++ b/net/core/net-sysfs.c
> > > @@ -1906,8 +1906,11 @@ static void netdev_queue_release(struct kobject *kobj)
> > >  {
> > >       struct netdev_queue *queue = to_netdev_queue(kobj);
> > >
> > > +     netdev_tracker_free(queue->dev, &queue->dev_tracker);
> > > +     /* Finish using the tracker before allowing the queue to be re-added. */
> > > +     smp_mb();
> >
> > Can't you use smp_wmb() instead as it's used to order two stores?
> 
> I do not think smp_wmb() would be enough: ref_tracker_free() only reads
> queue->dev_tracker, it never writes to it. We need to order a load
> before a store; smp_wmb() only orders stores.

Ah right, it's not a store. Thanks for checking!

> But what does this smp_mb() pair with in netdev_queue_add_kobject()?
> 
> The changelog mentions a control dependency, but netdev_hold() is not
> inside the if () clause, and state_initialized is a bitfield, so
> READ_ONCE() is not possible. This works thanks to the early return,
> but it is implicit and fragile.
> 
> Please add an explicit smp_mb() in netdev_queue_add_kobject() after
> the state_initialized check, with a comment pointing to
> netdev_queue_release(). This is not a fast path, and will help code
> review/understanding.
> 
> if (unlikely(kobj->state_initialized)) {
>         netdev_warn_once(dev, "Cannot re-add tx queues before their
> removal completed");
>         return -EAGAIN;
> }
> /* Pairs with smp_mb() in netdev_queue_release(): the previous
> * lifetime must be done with queue->dev_tracker before we reuse it.
> */
> smp_mb();

+1

> Also change ( /* Finish using the tracker before allowing the queue to
> be re-added. */)
> to the symmetric one (pairs with smp_mb() in  in netdev_queue_add_kobject()...)