netdev - Re: [PATCH v2] tracepoint: Do not fail unregistering a probe due to memory allocation

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]
Date:   Tue, 17 Nov 2020 20:54:24 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        Matt Mullins <mmullins@...x.us>, paulmck <paulmck@...nel.org>,
        Ingo Molnar <mingo@...hat.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        Andrii Nakryiko <andriin@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...omium.org>,
        netdev <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Kees Cook <keescook@...omium.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Josh Poimboeuf <jpoimboe@...hat.com>
Subject: Re: [PATCH v2] tracepoint: Do not fail unregistering a probe due to
 memory allocation

On Tue, Nov 17, 2020 at 6:18 PM Steven Rostedt <rostedt@...dmis.org> wrote:
>
> From: "Steven Rostedt (VMware)" <rostedt@...dmis.org>
>
> The list of tracepoint callbacks is managed by an array that is protected
> by RCU. To update this array, a new array is allocated, the updates are
> copied over to the new array, and then the list of functions for the
> tracepoint is switched over to the new array. After a completion of an RCU
> grace period, the old array is freed.
>
> This process happens for both adding a callback as well as removing one.
> But on removing a callback, if the new array fails to be allocated, the
> callback is not removed, and may be used after it is freed by the clients
> of the tracepoint.
>
> There's really no reason to fail if the allocation for a new array fails
> when removing a function. Instead, the function can simply be replaced by a
> stub that will be ignored in the callback loop, and it will be cleaned up
> on the next modification of the array.
>
> Link: https://lore.kernel.org/r/20201115055256.65625-1-mmullins@mmlx.us
> Link: https://lkml.kernel.org/r/20201116175107.02db396d@gandalf.local.home
>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
> Cc: Ingo Molnar <mingo@...hat.com>
> Cc: Alexei Starovoitov <ast@...nel.org>
> Cc: Daniel Borkmann <daniel@...earbox.net>
> Cc: Dmitry Vyukov <dvyukov@...gle.com>
> Cc: Martin KaFai Lau <kafai@...com>
> Cc: Song Liu <songliubraving@...com>
> Cc: Yonghong Song <yhs@...com>
> Cc: Andrii Nakryiko <andriin@...com>
> Cc: John Fastabend <john.fastabend@...il.com>
> Cc: KP Singh <kpsingh@...omium.org>
> Cc: netdev <netdev@...r.kernel.org>
> Cc: bpf <bpf@...r.kernel.org>
> Cc: Kees Cook <keescook@...omium.org>
> Cc: stable@...r.kernel.org
> Fixes: 97e1c18e8d17b ("tracing: Kernel Tracepoints")
> Reported-by: syzbot+83aa762ef23b6f0d1991@...kaller.appspotmail.com
> Reported-by: syzbot+d29e58bb557324e55e5e@...kaller.appspotmail.com
> Reported-by: Matt Mullins <mmullins@...x.us>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@...dmis.org>
> ---
> Changes since v1:
>    Use 1L value for stub function, and ignore calling it.
>
>  include/linux/tracepoint.h |  9 ++++-
>  kernel/tracepoint.c        | 80 +++++++++++++++++++++++++++++---------
>  2 files changed, 69 insertions(+), 20 deletions(-)
>
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index 0f21617f1a66..2e06e05b9d2a 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -33,6 +33,8 @@ struct trace_eval_map {
>
>  #define TRACEPOINT_DEFAULT_PRIO        10
>
> +#define TRACEPOINT_STUB                ((void *)0x1L)
> +
>  extern struct srcu_struct tracepoint_srcu;
>
>  extern int
> @@ -310,7 +312,12 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
>                 do {                                                    \
>                         it_func = (it_func_ptr)->func;                  \
>                         __data = (it_func_ptr)->data;                   \
> -                       ((void(*)(void *, proto))(it_func))(__data, args); \
> +                       /*                                              \
> +                        * Removed functions that couldn't be allocated \
> +                        * are replaced with TRACEPOINT_STUB.           \
> +                        */                                             \
> +                       if (likely(it_func != TRACEPOINT_STUB))         \
> +                               ((void(*)(void *, proto))(it_func))(__data, args); \

I think you're overreacting to the problem.
Adding run-time check to extremely unlikely problem seems wasteful.
99.9% of the time allocate_probes() will do kmalloc from slab of small
objects.
If that slab is out of memory it means it cannot allocate a single page.
In such case so many things will be failing to alloc that system
is unlikely operational. oom should have triggered long ago.
Imo Matt's approach to add __GFP_NOFAIL to allocate_probes()
when it's called from func_remove() is much better.
The error was reported by syzbot that was using
memory fault injections. ENOMEM in allocate_probes() was
never seen in real life and highly unlikely will ever be seen.