netdev - Re: [PATCH net] net: page_pool: allow enabling recycling late, fix false positive warning

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] [day] [month] [year] [list]

Message-ID: <aI0zVd1QJ-CMVX3u@mini-arch>
Date: Fri, 1 Aug 2025 14:36:21 -0700
From: Stanislav Fomichev <stfomichev@...il.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,
	David Wei <dw@...idwei.uk>, michael.chan@...adcom.com,
	pavan.chebbi@...adcom.com, hawk@...nel.org,
	ilias.apalodimas@...aro.org, almasrymina@...gle.com,
	sdf@...ichev.me
Subject: Re: [PATCH net] net: page_pool: allow enabling recycling late, fix
 false positive warning

On 08/01, Jakub Kicinski wrote:
> On Fri, 1 Aug 2025 13:55:09 -0700 Stanislav Fomichev wrote:
> > > +static void bnxt_enable_rx_page_pool(struct bnxt_rx_ring_info *rxr)
> > > +{
> > > +	page_pool_enable_direct_recycling(rxr->head_pool, &rxr->bnapi->napi);
> > > +	page_pool_enable_direct_recycling(rxr->page_pool, &rxr->bnapi->napi);  
> > 
> > We do bnxt_separate_head_pool check for the disable_direct_recycling
> > of head_pool. Is it safe to skip the check here because we always allocate two
> > pps from queue_mgmt callbacks? (not clear for me from a quick glance at
> > bnxt_alloc_rx_page_pool)
> 
> It's safe (I hope) because the helper is duplicate-call-friendly:
> 
> +void page_pool_enable_direct_recycling(struct page_pool *pool,
> +				       struct napi_struct *napi)
> +{
> +	if (READ_ONCE(pool->p.napi) == napi)   <<< right here
> +		return;
> +	WARN_ON_ONCE(!napi || pool->p.napi);
> +
> +	mutex_lock(&page_pools_lock);
> +	WRITE_ONCE(pool->p.napi, napi);
> +	mutex_unlock(&page_pools_lock);
> +}
> 
> We already have a refcount in page pool, I'm planning to add
> page_pool_get() in net-next and remove the
> 
> 	if (bnxt_separate_head_pool)
> 
> before page_pool_destroy(), too.

Ah, I see, I missed that fact that page_pool and head_pool point to the
same address when we don't have separate pools. Makes sense, thanks!