Thread (5 messages) 5 messages, 3 authors, 2021-01-27

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

From: Alexei Starovoitov <hidden>
Date: 2020-11-18 04:54:59
Also in: bpf, lkml

On Tue, Nov 17, 2020 at 6:18 PM Steven Rostedt [off-list ref] wrote:
quoted hunk ↗ jump to hunk
From: "Steven Rostedt (VMware)" <rostedt@goodmis.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 (local)
Link: https://lkml.kernel.org/r/20201116175107.02db396d@gandalf.local.home

Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Martin KaFai Lau <redacted>
Cc: Song Liu <redacted>
Cc: Yonghong Song <redacted>
Cc: Andrii Nakryiko <redacted>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: KP Singh <redacted>
Cc: netdev <redacted>
Cc: bpf <redacted>
Cc: Kees Cook <redacted>
Cc: stable@vger.kernel.org
Fixes: 97e1c18e8d17b ("tracing: Kernel Tracepoints")
Reported-by: syzbot+83aa762ef23b6f0d1991@syzkaller.appspotmail.com
Reported-by: syzbot+d29e58bb557324e55e5e@syzkaller.appspotmail.com
Reported-by: Matt Mullins <redacted>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.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.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help