netdev - Re: [PATCH net-next] page_pool: add a lockdep check for recycling in hardirq

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: <fc51f6be-819c-7b42-e0e7-4b474a690a8e@intel.com>
Date: Fri, 21 Jul 2023 18:33:39 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Jakub Kicinski <kuba@...nel.org>
CC: <davem@...emloft.net>, <netdev@...r.kernel.org>, <edumazet@...gle.com>,
	<pabeni@...hat.com>, <peterz@...radead.org>, <mingo@...hat.com>,
	<will@...nel.org>, <longman@...hat.com>, <boqun.feng@...il.com>,
	<hawk@...nel.org>, <ilias.apalodimas@...aro.org>
Subject: Re: [PATCH net-next] page_pool: add a lockdep check for recycling in
 hardirq

From: Jakub Kicinski <kuba@...nel.org>
Date: Fri, 21 Jul 2023 09:05:38 -0700

> On Fri, 21 Jul 2023 17:48:25 +0200 Alexander Lobakin wrote:
>>> Page pool use in hardirq is prohibited, add debug checks
>>> to catch misuses. IIRC we previously discussed using
>>> DEBUG_NET_WARN_ON_ONCE() for this, but there were concerns
>>> that people will have DEBUG_NET enabled in perf testing.
>>> I don't think anyone enables lockdep in perf testing,
>>> so use lockdep to avoid pushback and arguing :)  
>>
>> +1 patch to add to my tree to base my current series on...
>> Time to create separate repo named "page-pool-next"? :D
> 
> You joke but I've been scheming how to expose the page pool stats
> via the netdev netlink family, which would be another conflict to
> be added to the pile :D When it rains it pours.
> 
> You should probably start sending uncontroversial stuff out even
> if it doesn't have in-tree users yet.
> 
>>>  # define lockdep_assert_preemption_enabled() do { } while (0)
>>>  # define lockdep_assert_preemption_disabled() do { } while (0)
>>> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
>>> index a3e12a61d456..3ac760fcdc22 100644
>>> --- a/net/core/page_pool.c
>>> +++ b/net/core/page_pool.c
>>> @@ -536,6 +536,8 @@ static void page_pool_return_page(struct page_pool *pool, struct page *page)
>>>  static bool page_pool_recycle_in_ring(struct page_pool *pool, struct page *page)  
>>
>> Crap can happen earlier. Imagine that some weird code asked for direct
>> recycling with IRQs disabled. Then, we can hit
>> __page_pool_put_page:page_pool_recycle_in_cache and who knows what can
>> happen.
>> Can't we add this assertion right to the beginning of
>> __page_pool_put_page()? It's reasonable enough, at least for me, and
>> wouldn't require any commentary splats. Unlike put_defragged_page() as
>> Yunsheng proposes :p
>>
>> Other than that (which is debatable), looks fine to me.
> 
> No strong preference. Would you mind taking over this one? 
> It'd also benefit from testing that the lockdep warning actually 
> fires as expected, I just tested that it doesn't false positive TBH :)

Sure! I'll add it to the optimization series as a pre-req to more
aggressive direct recycling, would that be fine?

Other than that, it's mostly Yunsheng's 2 submissions (PP header file
split and hybrid allocation) I'm basing both of my series on. The
optimization series can go without the allocation just fine, but as for
the header split, I'd like it to go first, it simplifies things :D

Thanks,
Olek