netdev - Re: [PATCH net-next] net: Don't return pfmemalloc pages to the page pool.

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: <A7AC297F-066C-4AB0-81F7-D40BFC74BF20@gmail.com>
Date:   Thu, 20 Dec 2018 14:11:35 -0800
From:   "Jonathan Lemon" <jonathan.lemon@...il.com>
To:     "Jesper Dangaard Brouer" <brouer@...hat.com>
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH net-next] net: Don't return pfmemalloc pages to the page
 pool.

(Resending due to mailer issues)

On 20 Dec 2018, at 5:03, Jesper Dangaard Brouer wrote:

> On Wed, 19 Dec 2018 12:06:51 -0800
> Jonathan Lemon <jonathan.lemon@...il.com> wrote:
>
>> Return pfmemalloc pages back to the page allocator, instead of
>> holding them in the page pool.
>
> Have you experience this issue in practice or is it theory?

We're seeing the mlx5 driver use pfmemalloc pages with 4.11, and then 
return them
back to the page allocator.  (it's triggering the 
mlx5e_page_is_reserved() test).
The page pool code isn't in production use, but the code paths appear 
identical.

>
>>  While here, also use the __page_pool_return_page() API.
>
> Don't combine several unrelated changed in one patch.

Okay - will send as 2 separate patches

>> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
>> index 43a932cb609b..091007ff14a3 100644
>> --- a/net/core/page_pool.c
>> +++ b/net/core/page_pool.c
>> @@ -233,7 +233,7 @@ void __page_pool_put_page(struct page_pool *pool,
>>  	 *
>>  	 * refcnt == 1 means page_pool owns page, and can recycle it.
>>  	 */
>> -	if (likely(page_ref_count(page) == 1)) {
>> +	if (likely(page_ref_count(page) == 1 && !page_is_pfmemalloc(page))) 
>> {
>
> I don't like adding this in the hot-path.  Instead we could move this
> to the page alloc slow-path, and reject allocating pages with
> pgmemalloc in the first place.

No real objection to that - but then why bother with pfmemalloc?  If the 
driver can't
obtain pages for emergency use, then they might as well not exist.

--
Jonathan