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

[Alexei Starovoitov]

On Tue, Nov 17, 2020 at 6:18 PM Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>
> From: "Steven Rostedt (VMware)" <rostedt@xxxxxxxxxxx>
>
> 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@xxxxxxx
> Link: https://lkml.kernel.org/r/20201116175107.02db396d@xxxxxxxxxxxxxxxxxx
>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: Alexei Starovoitov <ast@xxxxxxxxxx>
> Cc: Daniel Borkmann <daniel@xxxxxxxxxxxxx>
> Cc: Dmitry Vyukov <dvyukov@xxxxxxxxxx>
> Cc: Martin KaFai Lau <kafai@xxxxxx>
> Cc: Song Liu <songliubraving@xxxxxx>
> Cc: Yonghong Song <yhs@xxxxxx>
> Cc: Andrii Nakryiko <andriin@xxxxxx>
> Cc: John Fastabend <john.fastabend@xxxxxxxxx>
> Cc: KP Singh <kpsingh@xxxxxxxxxxxx>
> Cc: netdev <netdev@xxxxxxxxxxxxxxx>
> Cc: bpf <bpf@xxxxxxxxxxxxxxx>
> Cc: Kees Cook <keescook@xxxxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: 97e1c18e8d17b ("tracing: Kernel Tracepoints")
> Reported-by: syzbot+83aa762ef23b6f0d1991@xxxxxxxxxxxxxxxxxxxxxxxxx
> Reported-by: syzbot+d29e58bb557324e55e5e@xxxxxxxxxxxxxxxxxxxxxxxxx
> Reported-by: Matt Mullins <mmullins@xxxxxxx>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@xxxxxxxxxxx>
> ---
> 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.