On Tue, Nov 17, 2020 at 6:18 PM Steven Rostedt <[email protected]> wrote: > > From: "Steven Rostedt (VMware)" <[email protected]> > > 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/[email protected] > Link: https://lkml.kernel.org/r/[email protected] > > Cc: Mathieu Desnoyers <[email protected]> > Cc: Ingo Molnar <[email protected]> > Cc: Alexei Starovoitov <[email protected]> > Cc: Daniel Borkmann <[email protected]> > Cc: Dmitry Vyukov <[email protected]> > Cc: Martin KaFai Lau <[email protected]> > Cc: Song Liu <[email protected]> > Cc: Yonghong Song <[email protected]> > Cc: Andrii Nakryiko <[email protected]> > Cc: John Fastabend <[email protected]> > Cc: KP Singh <[email protected]> > Cc: netdev <[email protected]> > Cc: bpf <[email protected]> > Cc: Kees Cook <[email protected]> > Cc: [email protected] > Fixes: 97e1c18e8d17b ("tracing: Kernel Tracepoints") > Reported-by: [email protected] > Reported-by: [email protected] > Reported-by: Matt Mullins <[email protected]> > Signed-off-by: Steven Rostedt (VMware) <[email protected]> > --- > 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.
