Re: [net-next v1 PATCH 1/2] xdp: revert forced mem allocator removal for page_pool

From: Jesper Dangaard Brouer <brouer@redhat.com>
To: "Jonathan Lemon" <jonathan.lemon@gmail.com>
Cc: "Toke Høiland-Jørgensen" <toke@redhat.com>,
	netdev@vger.kernel.org,
	"Ilias Apalodimas" <ilias.apalodimas@linaro.org>,
	"Saeed Mahameed" <saeedm@mellanox.com>,
	"Matteo Croce" <mcroce@redhat.com>,
	"Lorenzo Bianconi" <lorenzo@kernel.org>,
	"Tariq Toukan" <tariqt@mellanox.com>,
	brouer@redhat.com
Subject: Re: [net-next v1 PATCH 1/2] xdp: revert forced mem allocator removal for page_pool
Date: Sat, 9 Nov 2019 17:11:09 +0100	[thread overview]
Message-ID: <20191109171109.38c90490@carbon> (raw)
In-Reply-To: <80027E83-6C82-4238-AF7E-315F09457F43@gmail.com>

On Fri, 08 Nov 2019 11:16:43 -0800
"Jonathan Lemon" <jonathan.lemon@gmail.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