netdev - Re: [BUG] Possible unsafe page

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: <20230823133140.qbrxOLu-@linutronix.de>
Date: Wed, 23 Aug 2023 15:31:40 +0200
From: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To: Ilias Apalodimas <ilias.apalodimas@...aro.org>
Cc: netdev@...r.kernel.org, Ratheesh Kannoth <rkannoth@...vell.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Geetha sowjanya <gakula@...vell.com>,
	Jakub Kicinski <kuba@...nel.org>,
	Jesper Dangaard Brouer <hawk@...nel.org>,
	Paolo Abeni <pabeni@...hat.com>,
	Subbaraya Sundeep <sbhatta@...vell.com>,
	Sunil Goutham <sgoutham@...vell.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	hariprasad <hkelam@...vell.com>
Subject: Re: [BUG] Possible unsafe page_pool usage in octeontx2

On 2023-08-23 14:36:06 [+0300], Ilias Apalodimas wrote:
> Hi Sebastian,
Hi Ilias,

> >                                                       dma_sync_size);
> >
> > -               if (allow_direct && in_softirq() &&
> > +               if (allow_direct && in_serving_softirq() &&
> >                     page_pool_recycle_in_cache(page, pool))
> >                         return NULL;
> >
> 
> FWIW we used to have that check.
> commit 542bcea4be866b ("net: page_pool: use in_softirq() instead")
> changed that, so maybe we should revert that overall?

The other changes look okay, this in particular I am not sure about. It
would end up in the pool->ring instead of the lock-less cache. It still
depends how it got here. If it comes from page_pool_return_skb_page()
then the list_owner check will fail because it is not set for the
threaded-NAPI case. If it was a real concern, the entry point must have
been page_pool_put_full_page() or later. This basically assumes the same
context.

> > because the intention (as I understand it) is to be invoked from within
> > the NAPI callback (while softirq is served) and not if BH is just
> > disabled due to a lock or so.
> >
> > It would also make sense to a add WARN_ON_ONCE(!in_serving_softirq()) to
> > page_pool_alloc_pages() to spot usage outside of softirq. But this will
> > trigger in every driver since the same function is used in the open
> > callback to initially setup the HW.
> 
> What about adding a check in the cached allocation path in order to
> skip the initial page allocation?

Maybe. I'm a bit worried that this lock-less has no lockdep or similar
checks if everyone plays by the rules.

> Thanks
> /Ilias

Sebastian