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

[Björn Töpel]

Stanislav Fomichev <sdf.kernel@xxxxxxxxx> writes:

> 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.

...more PM suspend/resume issues...

I'd say this a bad smell/poor XSK contract that we need to use wq
freezable here. XSK capable drivers (and the core) should tolerate
calling the pool-removal path on suspended hardware and failed
resume/unregister.

Now that poor design leaks into net_device. :/

>> 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) ?

+1

> 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.

+1

I've made a mental note to try to improve this, and hopefully the we can
get rid of this in the future.


Björn