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]

Date: Fri, 25 Aug 2023 15:38:54 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Jesper Dangaard Brouer <hawk@...nel.org>
CC: Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
	<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>, Ilias Apalodimas
	<ilias.apalodimas@...aro.org>, Jakub Kicinski <kuba@...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>, Qingfang DENG <qingfang.deng@...lower.com.cn>
Subject: Re: [BUG] Possible unsafe page_pool usage in octeontx2

From: Jesper Dangaard Brouer <hawk@...nel.org>
Date: Fri, 25 Aug 2023 15:22:05 +0200

> 
> 
> On 24/08/2023 17.26, Alexander Lobakin wrote:
>> From: Jesper Dangaard Brouer<hawk@...nel.org>
>> Date: Wed, 23 Aug 2023 21:45:04 +0200
>>
>>> (Cc Olek as he have changes in this code path)
>> Thanks! I was reading the thread a bit on LKML, but being in the CC list
>> is more convenient :D
>>
> 
> :D
> 
>>> On 23/08/2023 11.47, Sebastian Andrzej Siewior wrote:
>>>> Hi,
>>>>
>>>> I've been looking at the page_pool locking.
>>>>
>>>> page_pool_alloc_frag() -> page_pool_alloc_pages() ->
>>>> __page_pool_get_cached():
>>>>
>>>> There core of the allocation is:
>>>> |         /* Caller MUST guarantee safe non-concurrent access, e.g.
>>>> softirq */
>>>> |         if (likely(pool->alloc.count)) {
>>>> |                 /* Fast-path */
>>>> |                 page = pool->alloc.cache[--pool->alloc.count];
>>>>
>>>> The access to the `cache' array and the `count' variable is not locked.
>>>> This is fine as long as there only one consumer per pool. In my
>>>> understanding the intention is to have one page_pool per NAPI callback
>>>> to ensure this.
>>>>
>>> Yes, the intention is a single PP instance is "bound" to one RX-NAPI.
>>
>> Isn't that also a misuse of page_pool->p.napi? I thought it can be set
>> only when page allocation and cache refill happen both inside the same
>> NAPI polling function. Otx2 uses workqueues to refill the queues,
>> meaning that consumer and producer can happen in different contexts or
>> even threads and it shouldn't set p.napi.
>>
> 
> As Jakub wrote this otx2 driver doesn't set p.napi (so that part of the
> problem cannot happen).

Oh I'm dumb :z

> 
> That said using workqueues to refill the queues is not compatible with
> using page_pool_alloc APIs.  This need to be fixed in driver!

Hmmm I'm wondering now, how should we use Page Pool if we want to run
consume and alloc routines separately? Do you want to say we can't use
Page Pool in that case at all? Quoting other your reply:

> This WQ process is not allowed to use the page_pool_alloc() API this
> way (from a work-queue).  The PP alloc-side API must only be used
> under NAPI protection.

Who did say that? If I don't set p.napi, how is Page Pool then tied to NAPI?
Moreover, when you initially do an ifup/.ndo_open, you have to fill your
Rx queues. It's process context and it can happen on whichever CPU.
Do you mean I can't allocate pages in .ndo_open? :D

> 
> --Jesper

Thanks,
Olek