Re: [PATCH net v3] xsk: freeze deferred pool teardown without blocking unregister

[Stanislav Fomichev <sdf.kernel@gmail.com>]

On 10/13, James Hilliard wrote:
> Deferred pool destruction calls ndo_bpf() under RTNL. system_wq is
> not frozen during system sleep, so that callback can run after a
> device has suspended and gated its clocks. Use system_freezable_wq
> so running destruction finishes before device suspend and new work
> waits until process thaw.
> 
> Keep assigned pools visible to NETDEV_UNREGISTER independently of
> the socket list. A released socket has already left that list, but
> its final pool put can queue destruction after workqueues freeze.
> A resume-time unregister would then wait for a device reference
> whose release cannot run until the resume completes.
> 
> Track assigned pools per netdev with netdev operations locking and detach
> remaining pools after the notifier socket walk, including copy-mode pools.
> This also covers leased queues without scanning pools from unrelated
> devices or network namespaces. Remove the entry on assignment failure and
> normal teardown. The deferred worker still owns the pool and later observes
> the cleared device pointer, avoiding a second driver detach or put.
> 
> The lifetime problem was identified by code inspection of the deferred
> release and system-sleep paths.
> 
> Fixes: 1c1efc2af158 ("xsk: Create and free buffer pool independently from umem")
> Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> ---
> Changes in v3:
> - Protect the per-netdev pool list with netdev operations locking, as
>   requested by Stanislav Fomichev and Björn Töpel.
> - Assert operations locking for copy-mode assignment too, and share a
>   locked detach helper so unregister holds the lock across the pool walk.
> - Rebase onto current net.
> - Link to v2: https://patch.msgid.link/20261005-xsk-suspend-teardown-v2-1-2d87228f4329@gmail.com
> 
> Changes in v2:
> - Track assigned pools per netdev instead of scanning a global pool list.
> - Keep deferred releases visible across queue changes and queue leases.
> - Rebase onto current net.
> - Link to v1: https://patch.msgid.link/20260930-xsk-suspend-teardown-v1-1-a6cac8c030be@gmail.com
> ---
>  include/linux/netdevice.h   |  5 +++++
>  include/net/xsk_buff_pool.h |  3 +++
>  net/xdp/xsk.c               |  5 +++++
>  net/xdp/xsk_buff_pool.c     | 46 ++++++++++++++++++++++++++++++++++++++-------
>  4 files changed, 52 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 3cff2174dc03..ac257221c7c5 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -2545,6 +2545,11 @@ struct net_device {
>  	/* protected by rtnl_lock */
>  	struct bpf_xdp_entity	xdp_state[__MAX_XDP_MODE];
>  
> +#ifdef CONFIG_XDP_SOCKETS
> +	/** @xsk_pools: assigned AF_XDP pools, ops protected */
> +	struct hlist_head	xsk_pools;
> +#endif
> +
>  	u8 dev_addr_shadow[MAX_ADDR_LEN];
>  	netdevice_tracker	linkwatch_dev_tracker;
>  	netdevice_tracker	watchdog_dev_tracker;
> diff --git a/include/net/xsk_buff_pool.h b/include/net/xsk_buff_pool.h
> index a7df573784fd..02e89350faf9 100644
> --- a/include/net/xsk_buff_pool.h
> +++ b/include/net/xsk_buff_pool.h
> @@ -48,6 +48,8 @@ struct xsk_buff_pool {
>  	struct device *dev;
>  	struct net_device *netdev;
>  	struct list_head xsk_tx_list;
> +	/* Assigned pools, including deferred releases; ops protected. */
> +	struct hlist_node dev_node;
>  	/* Protects modifications to the xsk_tx_list */
>  	spinlock_t xsk_tx_list_lock;
>  	refcount_t users;
> @@ -119,6 +121,7 @@ bool xp_put_pool(struct xsk_buff_pool *pool);
>  void xp_clear_dev(struct xsk_buff_pool *pool);
>  void xp_add_xsk(struct xsk_buff_pool *pool, struct xdp_sock *xs);
>  void xp_del_xsk(struct xsk_buff_pool *pool, struct xdp_sock *xs);
> +void xp_clear_dev_all(struct net_device *dev);
>  
>  /* AF_XDP, and XDP core. */
>  void xp_free(struct xdp_buff_xsk *xskb);
> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index 33475b180ea6..781ba46eb152 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -2145,6 +2145,11 @@ static int xsk_notifier(struct notifier_block *this,
>  			mutex_unlock(&xs->mutex);
>  		}
>  		mutex_unlock(&net->xdp.lock);
> +		/*
> +		 * A released socket is no longer on xdp.list. Its pool can still
> +		 * hold a device reference on the frozen release workqueue.
> +		 */
> +		xp_clear_dev_all(dev);
>  		break;
>  	}
>  	return NOTIFY_DONE;
> diff --git a/net/xdp/xsk_buff_pool.c b/net/xdp/xsk_buff_pool.c
> index 9d2d94f1fb75..5b9dcafd6882 100644
> --- a/net/xdp/xsk_buff_pool.c
> +++ b/net/xdp/xsk_buff_pool.c
> @@ -173,6 +173,7 @@ int xp_assign_dev(struct xsk_buff_pool *pool,
>  	int err = 0;
 

[..]

>  	ASSERT_RTNL();
> +	netdev_assert_locked_ops_compat(netdev);

nit: can now drop ASSERT_RTNL? netdev_assert_locked_ops_compat should be
enough.

>  
>  	force_zc = flags & XDP_ZEROCOPY;
>  	force_copy = flags & XDP_COPY;
> @@ -204,6 +205,11 @@ int xp_assign_dev(struct xsk_buff_pool *pool,
>  	pool->cached_need_wakeup = XDP_WAKEUP_TX;
>  
>  	dev_hold(netdev);
> +	/*
> +	 * Socket removal precedes the final pool put. Keep the pool visible to
> +	 * NETDEV_UNREGISTER even while release work is waiting for process thaw.
> +	 */
> +	hlist_add_head(&pool->dev_node, &netdev->xsk_pools);
>  
>  	if (force_copy)
>  		/* For copy-mode, we are done. */
> @@ -244,7 +250,6 @@ int xp_assign_dev(struct xsk_buff_pool *pool,
>  	bpf.xsk.pool = pool;
>  	bpf.xsk.queue_id = queue_id;
>  
> -	netdev_assert_locked_ops_compat(netdev);
>  	err = netdev->netdev_ops->ndo_bpf(netdev, &bpf);
>  	if (err)
>  		goto err_unreg_pool;
> @@ -265,6 +270,8 @@ int xp_assign_dev(struct xsk_buff_pool *pool,
>  		err = 0; /* fallback to copy mode */
>  	if (err) {
>  		xsk_clear_pool_at_qid(netdev, queue_id);
> +		hlist_del(&pool->dev_node);
> +		pool->netdev = NULL;
>  		dev_put(netdev);
>  	}
>  	return err;
> @@ -287,19 +294,40 @@ int xp_assign_dev_shared(struct xsk_buff_pool *pool, struct xdp_sock *umem_xs,
>  	return xp_assign_dev(pool, dev, queue_id, flags);
>  }
>  
> +static void __xp_clear_dev(struct xsk_buff_pool *pool)
> +{
> +	struct net_device *netdev = pool->netdev;
> +
> +	netdev_assert_locked_ops_compat(netdev);
> +	xp_disable_drv_zc(pool);
> +	xsk_clear_pool_at_qid(netdev, pool->queue_id);
> +	hlist_del(&pool->dev_node);
> +	pool->netdev = NULL;
> +	dev_put(netdev);
> +}
> +
>  void xp_clear_dev(struct xsk_buff_pool *pool)
>  {
>  	struct net_device *netdev = pool->netdev;
>  
> -	if (!pool->netdev)
> +	if (!netdev)
>  		return;
>  
>  	netdev_lock_ops(netdev);
> -	xp_disable_drv_zc(pool);
> -	xsk_clear_pool_at_qid(pool->netdev, pool->queue_id);
> -	pool->netdev = NULL;
> +	__xp_clear_dev(pool);
>  	netdev_unlock_ops(netdev);
> -	dev_put(netdev);
> +}
> +
> +void xp_clear_dev_all(struct net_device *dev)
> +{
> +	struct xsk_buff_pool *pool;
> +	struct hlist_node *next;
> +

[..]

> +	netdev_lock_ops(dev);
> +	netdev_assert_locked_ops_compat(dev);

And here also no need for netdev_assert_locked_ops_compat right after
netdev_lock_ops. You have one in __xp_clear_dev, that should be enough.
With that:

Acked-by: Stanislav Fomichev <sdf@fomichev.me>