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.
