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

["Jonathan Lemon" <jonathan.lemon@gmail.com>]

On 8 Nov 2019, at 10:20, Jesper Dangaard Brouer wrote:

> Forced removal of XDP mem allocator, specifically related to 
> page_pool, turned
> out to be a wrong approach.  Special thanks to Jonathan Lemon for 
> convincing me.
> This patch is a partial revert of commit d956a048cd3f (“xdp: force 
> mem allocator
> removal and periodic warning”).
>
> It is much better to provide a guarantee that page_pool object stays 
> valid
> until 'inflight' pages reach zero, making it safe to remove.
>
> We keep the periodic warning via a work-queue, but increased interval 
> to
> 5-minutes. The reason is to have a way to catch bugs, where inflight
> pages/packets never reach zero, indicating some kind of leak. These 
> kind of
> bugs have been observed while converting drivers over to use page_pool 
> API.
>
> Details on when to crash the kernel. If page_pool API is misused and
> somehow __page_pool_free() is invoked while there are still inflight
> frames, then (like before) a WARN() is triggered and not a BUG(). This 
> can
> potentially lead to use-after-free, which we try to catch via 
> poisoning the
> page_pool object memory with some NULL pointers. Doing it this way,
> pinpoint both the driver (likely) prematurely freeing page_pool via 
> WARN(),
> and crash-dump for inflight page/packet show who to blame for late 
> return.
>
> Fixes: d956a048cd3f (“xdp: force mem allocator removal and periodic 
> warning”)
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> ---
>  include/trace/events/xdp.h |   35 +++--------------------------------
>  net/core/page_pool.c       |    8 ++++++--
>  net/core/xdp.c             |   36 
> +++++++++++++-----------------------
>  3 files changed, 22 insertions(+), 57 deletions(-)
>
> diff --git a/include/trace/events/xdp.h b/include/trace/events/xdp.h
> index c7e3c9c5bad3..a3ead2b1f00e 100644
> --- a/include/trace/events/xdp.h
> +++ b/include/trace/events/xdp.h
> @@ -318,9 +318,9 @@ __MEM_TYPE_MAP(__MEM_TYPE_TP_FN)
>  TRACE_EVENT(mem_disconnect,
>
>  	TP_PROTO(const struct xdp_mem_allocator *xa,
> -		 bool safe_to_remove, bool force),
> +		 bool safe_to_remove),
>
> -	TP_ARGS(xa, safe_to_remove, force),
> +	TP_ARGS(xa, safe_to_remove),
>
>  	TP_STRUCT__entry(
>  		__field(const struct xdp_mem_allocator *,	xa)
> @@ -328,7 +328,6 @@ TRACE_EVENT(mem_disconnect,
>  		__field(u32,		mem_type)
>  		__field(const void *,	allocator)
>  		__field(bool,		safe_to_remove)
> -		__field(bool,		force)
>  		__field(int,		disconnect_cnt)
>  	),
>
> @@ -338,17 +337,15 @@ TRACE_EVENT(mem_disconnect,
>  		__entry->mem_type	= xa->mem.type;
>  		__entry->allocator	= xa->allocator;
>  		__entry->safe_to_remove	= safe_to_remove;
> -		__entry->force		= force;
>  		__entry->disconnect_cnt	= xa->disconnect_cnt;
>  	),
>
>  	TP_printk("mem_id=%d mem_type=%s allocator=%p"
> -		  " safe_to_remove=%s force=%s disconnect_cnt=%d",
> +		  " safe_to_remove=%s disconnect_cnt=%d",
>  		  __entry->mem_id,
>  		  __print_symbolic(__entry->mem_type, __MEM_TYPE_SYM_TAB),
>  		  __entry->allocator,
>  		  __entry->safe_to_remove ? "true" : "false",
> -		  __entry->force ? "true" : "false",
>  		  __entry->disconnect_cnt
>  	)
>  );
> @@ -387,32 +384,6 @@ TRACE_EVENT(mem_connect,
>  	)
>  );
>
> -TRACE_EVENT(mem_return_failed,
> -
> -	TP_PROTO(const struct xdp_mem_info *mem,
> -		 const struct page *page),
> -
> -	TP_ARGS(mem, page),
> -
> -	TP_STRUCT__entry(
> -		__field(const struct page *,	page)
> -		__field(u32,		mem_id)
> -		__field(u32,		mem_type)
> -	),
> -
> -	TP_fast_assign(
> -		__entry->page		= page;
> -		__entry->mem_id		= mem->id;
> -		__entry->mem_type	= mem->type;
> -	),
> -
> -	TP_printk("mem_id=%d mem_type=%s page=%p",
> -		  __entry->mem_id,
> -		  __print_symbolic(__entry->mem_type, __MEM_TYPE_SYM_TAB),
> -		  __entry->page
> -	)
> -);
> -
>  #endif /* _TRACE_XDP_H */
>
>  #include <trace/define_trace.h>
> 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);
>  }
> @@ -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.



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


> +
>  	if (pool->p.flags & PP_FLAG_DMA_MAP)
>  		put_device(pool->p.dev);
>
> diff --git a/net/core/xdp.c b/net/core/xdp.c
> index 20781ad5f9c3..8673f199d9f4 100644
> --- a/net/core/xdp.c
> +++ b/net/core/xdp.c
> @@ -85,7 +85,7 @@ static void __xdp_mem_allocator_rcu_free(struct 
> rcu_head *rcu)
>  	kfree(xa);
>  }
>
> -static bool __mem_id_disconnect(int id, bool force)
> +static bool __mem_id_disconnect(int id)
>  {
>  	struct xdp_mem_allocator *xa;
>  	bool safe_to_remove = true;
> @@ -104,30 +104,26 @@ static bool __mem_id_disconnect(int id, bool 
> force)
>  	if (xa->mem.type == MEM_TYPE_PAGE_POOL)
>  		safe_to_remove = page_pool_request_shutdown(xa->page_pool);
>
> -	trace_mem_disconnect(xa, safe_to_remove, force);
> +	trace_mem_disconnect(xa, safe_to_remove);
>
> -	if ((safe_to_remove || force) &&
> +	if ((safe_to_remove) &&

Remove extra parenthesis.
-- 
Jonathan