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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e2d61c173d1a8f644d2682e9a0670ccdaa7deb81.camel@mellanox.com>
Date:   Thu, 20 Dec 2018 23:41:47 +0000
From:   Saeed Mahameed <saeedm@...lanox.com>
To:     "brouer@...hat.com" <brouer@...hat.com>,
        "jonathan.lemon@...il.com" <jonathan.lemon@...il.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next] net: Don't return pfmemalloc pages to the page
 pool.

On Thu, 2018-12-20 at 14:11 -0800, Jonathan Lemon wrote:
> (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 think this is wrong, if refcount is 1, then this page belongs to
pagepool and you must enter this statement's true block, and test
page_is_pfmemalloc inside (mark it unlikely),
to return a pfmemalloc page, you need to call __page_pool_return_page()
to dma_unmap and other cleanups if required.

> > 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.
> 

I prefer to enhance the above solution, no need to risk pagepool users
going out of memory under emergencies.

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