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

[Stanislav Fomichev]

On 10/05, 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 under RTNL 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@xxxxxxxxx>
> ---
> 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@xxxxxxxxx
> 
> To: "David S. Miller" <davem@xxxxxxxxxxxxx>
> To: Eric Dumazet <edumazet@xxxxxxxxxx>
> To: Jakub Kicinski <kuba@xxxxxxxxxx>
> To: Paolo Abeni <pabeni@xxxxxxxxxx>
> To: Simon Horman <horms@xxxxxxxxxx>
> To: Andrew Lunn <andrew+netdev@xxxxxxx>
> To: Magnus Karlsson <magnus.karlsson@xxxxxxxxx>
> To: Maciej Fijalkowski <maciej.fijalkowski@xxxxxxxxx>
> To: Stanislav Fomichev <sdf@xxxxxxxxxxx>
> To: Alexei Starovoitov <ast@xxxxxxxxxx>
> To: Daniel Borkmann <daniel@xxxxxxxxxxxxx>
> To: Jesper Dangaard Brouer <hawk@xxxxxxxxxx>
> To: John Fastabend <john.fastabend@xxxxxxxxx>
> To: Björn Töpel <bjorn@xxxxxxxxxx>
> Cc: netdev@xxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> Cc: bpf@xxxxxxxxxxxxxxx
> ---
>  include/linux/netdevice.h   |  5 +++++
>  include/net/xsk_buff_pool.h |  3 +++
>  net/xdp/xsk.c               |  5 +++++
>  net/xdp/xsk_buff_pool.c     | 25 ++++++++++++++++++++++++-
>  4 files changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 3cff2174dc03..72091938f6e6 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, protected by rtnl_lock */


Can we mark this as being ops protected (net_device::lock) ?
xp_clear_dev calls netdev_lock_ops, but xp_assign_dev
has netdev_assert_locked_ops_compat, so maybe there needs to be a bit more
care. We don't want to add new ASSERT_RTNL if possible (and extend new
netdev lock semantics).

The rest looks good.