| 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
| ||
|
Message-ID: <80027E83-6C82-4238-AF7E-315F09457F43@gmail.com>
Date: Fri, 08 Nov 2019 11:16:43 -0800
From: "Jonathan Lemon" <jonathan.lemon@...il.com>
To: "Jesper Dangaard Brouer" <brouer@...hat.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>
Subject: Re: [net-next v1 PATCH 1/2] xdp: revert forced mem allocator removal
for page_pool
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@...hat.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
Powered by blists - more mailing lists