netdev - Re: [PATCH net-next 2/8] net: designate XSK pool pointers in queues as "ops protected"

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z_SHQJ_pLOgz9vpM@LQ3V64L9R2>
Date: Mon, 7 Apr 2025 19:17:36 -0700
From: Joe Damato <jdamato@...tly.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com,
	pabeni@...hat.com, andrew+netdev@...n.ch, horms@...nel.org,
	sdf@...ichev.me, hramamurthy@...gle.com, kuniyu@...zon.com
Subject: Re: [PATCH net-next 2/8] net: designate XSK pool pointers in queues
 as "ops protected"

On Mon, Apr 07, 2025 at 12:01:11PM -0700, Jakub Kicinski wrote:
> Read accesses go via xsk_get_pool_from_qid(), the call coming
> from the core and gve look safe (other "ops locked" drivers
> don't support XSK).
> 
> Write accesses go via xsk_reg_pool_at_qid() and xsk_clear_pool_at_qid().
> Former is already under the ops lock, latter needs to be locked when
> coming from the workqueue via xp_clear_dev().
> 
> Acked-by: Stanislav Fomichev <sdf@...ichev.me>
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> Signed-off-by: Stanislav Fomichev <sdf@...ichev.me>
> ---
>  include/linux/netdevice.h     | 1 +
>  include/net/netdev_rx_queue.h | 6 +++---
>  net/xdp/xsk.c                 | 2 ++
>  net/xdp/xsk_buff_pool.c       | 7 ++++++-
>  4 files changed, 12 insertions(+), 4 deletions(-)

[...]

> diff --git a/net/xdp/xsk_buff_pool.c b/net/xdp/xsk_buff_pool.c
> index 25a76c5ce0f1..c7e50fd86c6a 100644
> --- a/net/xdp/xsk_buff_pool.c
> +++ b/net/xdp/xsk_buff_pool.c
> @@ -279,9 +279,14 @@ static void xp_release_deferred(struct work_struct *work)
>  {
>  	struct xsk_buff_pool *pool = container_of(work, struct xsk_buff_pool,
>  						  work);
> +	struct net_device *netdev = pool->netdev;
>  
>  	rtnl_lock();
> -	xp_clear_dev(pool);
> +	if (netdev) {
> +		netdev_lock_ops(netdev);
> +		xp_clear_dev(pool);
> +		netdev_unlock_ops(netdev);
> +	}
>  	rtnl_unlock();

Is it actually possible for netdev to be NULL here?

I feel like it probably isn't, but if it were possible we'd need an
else case here to xp_clear_dev(pool) without the netdev_lock_ops?