netdev - Re: [net-next v1 PATCH 1/2] xdp: revert forced mem allocator removal for 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: <20191109171109.38c90490@carbon>
Date:   Sat, 9 Nov 2019 17:11:09 +0100
From:   Jesper Dangaard Brouer <brouer@...hat.com>
To:     "Jonathan Lemon" <jonathan.lemon@...il.com>
Cc:     "Toke Høiland-Jørgensen" <toke@...hat.com>,
        netdev@...r.kernel.org,
        "Ilias Apalodimas" <ilias.apalodimas@...aro.org>,
        "Saeed Mahameed" <saeedm@...lanox.com>,
        "Matteo Croce" <mcroce@...hat.com>,
        "Lorenzo Bianconi" <lorenzo@...nel.org>,
        "Tariq Toukan" <tariqt@...lanox.com>, brouer@...hat.com
Subject: Re: [net-next v1 PATCH 1/2] xdp: revert forced mem allocator
 removal for page_pool

On Fri, 08 Nov 2019 11:16:43 -0800
"Jonathan Lemon" <jonathan.lemon@...il.com> wrote:

> > diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> > index 5bc65587f1c4..226f2eb30418 100644
> > --- a/net/core/page_pool.c
> > +++ b/net/core/page_pool.c
> > @@ -346,7 +346,7 @@ static void __warn_in_flight(struct page_pool 
> > *pool)
> >
> >  	distance = _distance(hold_cnt, release_cnt);
> >
> > -	/* Drivers should fix this, but only problematic when DMA is used */
> > +	/* BUG but warn as kernel should crash later */
> >  	WARN(1, "Still in-flight pages:%d hold:%u released:%u",
> >  	     distance, hold_cnt, release_cnt);

Because this is kept as a WARN, I set pool->ring.queue = NULL later.

> >  }
> > @@ -360,12 +360,16 @@ void __page_pool_free(struct page_pool *pool)
> >  	WARN(pool->alloc.count, "API usage violation");
> >  	WARN(!ptr_ring_empty(&pool->ring), "ptr_ring is not empty");
> >
> > -	/* Can happen due to forced shutdown */
> >  	if (!__page_pool_safe_to_destroy(pool))
> >  		__warn_in_flight(pool);  
> 
> If it's not safe to destroy, we shouldn't be getting here.

Don't make such assumptions. The API is going to be used by driver
developer and they are always a little too creative...

The page_pool is a separate facility, it is not tied to the
xdp_rxq_info memory model.  Some drivers use page_pool directly e.g.
drivers/net/ethernet/stmicro/stmmac.  It can easily trigger this case,
when some extend that driver.

 
> >  	ptr_ring_cleanup(&pool->ring, NULL);
> >
> > +	/* Make sure kernel will crash on use-after-free */
> > +	pool->ring.queue = NULL;
> > +	pool->alloc.cache[PP_ALLOC_CACHE_SIZE - 1] = NULL;
> > +	pool->alloc.count = PP_ALLOC_CACHE_SIZE;  
> 
> The pool is going to be freed.  This is useless code; if we're
> really concerned about use-after-free, the correct place for catching
> this is with the memory-allocator tools, not scattering things like
> this ad-hoc over the codebase.

No, I need this code here, because we kept the above WARN() and didn't
change that into a BUG().  It is obviously not a full solution for
use-after-free detection.  The memory subsystem have kmemleak to catch
this kind of stuff, but nobody runs this in production.  I need this
here to catch some obvious runtime cases.

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer