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: <20230721090538.57cfd15d@kernel.org>
Date: Fri, 21 Jul 2023 09:05:38 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Alexander Lobakin <aleksander.lobakin@...el.com>
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

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