On Wed, Nov 18, 2020 at 09:34:05AM -0500, Steven Rostedt 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 function that could be cleaned up on the next modification of the > array. That is, instead of calling the function registered to the > tracepoint, it would call a stub function in its place. > > Link: https://lore.kernel.org/r/20201115055256.65625-1-mmullins@xxxxxxx > Link: https://lore.kernel.org/r/20201116175107.02db396d@xxxxxxxxxxxxxxxxxx > Link: https://lore.kernel.org/r/20201117211836.54acaef2@xxxxxxxxxxxxxxxx > > 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: Florian Weimer <fw@xxxxxxxxxxxxx> > 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> I'm a bit late answering your initial query, but yes indeed this fixes the bug I was hunting. I just watched it live through the reproducer for about a half-hour, while unpatched I get an instant "BUG: unable to handle page fault". Tested-by: Matt Mullins <mmullins@xxxxxxx> > --- > Changes since v2: > - Went back to using a stub function and not touching > the fast path. > - Removed adding __GFP_NOFAIL from the allocation of the removal. > > kernel/tracepoint.c | 80 ++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 64 insertions(+), 16 deletions(-) > > diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c > index 3f659f855074..3e261482296c 100644 > --- a/kernel/tracepoint.c > +++ b/kernel/tracepoint.c > @@ -53,6 +53,12 @@ struct tp_probes { > struct tracepoint_func probes[]; > }; > > +/* Called in removal of a func but failed to allocate a new tp_funcs */ > +static void tp_stub_func(void) > +{ > + return; > +} > + > static inline void *allocate_probes(int count) > { > struct tp_probes *p = kmalloc(struct_size(p, probes, count), > @@ -131,6 +137,7 @@ func_add(struct tracepoint_func **funcs, struct tracepoint_func *tp_func, > { > struct tracepoint_func *old, *new; > int nr_probes = 0; > + int stub_funcs = 0; > int pos = -1; > > if (WARN_ON(!tp_func->func)) > @@ -147,14 +154,34 @@ func_add(struct tracepoint_func **funcs, struct tracepoint_func *tp_func, > if (old[nr_probes].func == tp_func->func && > old[nr_probes].data == tp_func->data) > return ERR_PTR(-EEXIST); > + if (old[nr_probes].func == tp_stub_func) > + stub_funcs++; > } > } > - /* + 2 : one for new probe, one for NULL func */ > - new = allocate_probes(nr_probes + 2); > + /* + 2 : one for new probe, one for NULL func - stub functions */ > + new = allocate_probes(nr_probes + 2 - stub_funcs); > if (new == NULL) > return ERR_PTR(-ENOMEM); > if (old) { > - if (pos < 0) { > + if (stub_funcs) { > + /* Need to copy one at a time to remove stubs */ > + int probes = 0; > + > + pos = -1; > + for (nr_probes = 0; old[nr_probes].func; nr_probes++) { > + if (old[nr_probes].func == tp_stub_func) > + continue; > + if (pos < 0 && old[nr_probes].prio < prio) > + pos = probes++; > + new[probes++] = old[nr_probes]; > + } > + nr_probes = probes; > + if (pos < 0) > + pos = probes; > + else > + nr_probes--; /* Account for insertion */ > + > + } else if (pos < 0) { > pos = nr_probes; > memcpy(new, old, nr_probes * sizeof(struct tracepoint_func)); > } else { > @@ -188,8 +215,9 @@ static void *func_remove(struct tracepoint_func **funcs, > /* (N -> M), (N > 1, M >= 0) probes */ > if (tp_func->func) { > for (nr_probes = 0; old[nr_probes].func; nr_probes++) { > - if (old[nr_probes].func == tp_func->func && > - old[nr_probes].data == tp_func->data) > + if ((old[nr_probes].func == tp_func->func && > + old[nr_probes].data == tp_func->data) || > + old[nr_probes].func == tp_stub_func) > nr_del++; > } > } > @@ -208,14 +236,32 @@ static void *func_remove(struct tracepoint_func **funcs, > /* N -> M, (N > 1, M > 0) */ > /* + 1 for NULL */ > new = allocate_probes(nr_probes - nr_del + 1); > - if (new == NULL) > - return ERR_PTR(-ENOMEM); > - for (i = 0; old[i].func; i++) > - if (old[i].func != tp_func->func > - || old[i].data != tp_func->data) > - new[j++] = old[i]; > - new[nr_probes - nr_del].func = NULL; > - *funcs = new; > + if (new) { > + for (i = 0; old[i].func; i++) > + if ((old[i].func != tp_func->func > + || old[i].data != tp_func->data) > + && old[i].func != tp_stub_func) > + new[j++] = old[i]; > + new[nr_probes - nr_del].func = NULL; > + *funcs = new; > + } else { > + /* > + * Failed to allocate, replace the old function > + * with calls to tp_stub_func. > + */ > + for (i = 0; old[i].func; i++) > + if (old[i].func == tp_func->func && > + old[i].data == tp_func->data) { > + old[i].func = tp_stub_func; > + /* Set the prio to the next event. */ > + if (old[i + 1].func) > + old[i].prio = > + old[i + 1].prio; > + else > + old[i].prio = -1; > + } > + *funcs = old; > + } > } > debug_print_probes(*funcs); > return old; > @@ -295,10 +341,12 @@ static int tracepoint_remove_func(struct tracepoint *tp, > tp_funcs = rcu_dereference_protected(tp->funcs, > lockdep_is_held(&tracepoints_mutex)); > old = func_remove(&tp_funcs, func); > - if (IS_ERR(old)) { > - WARN_ON_ONCE(PTR_ERR(old) != -ENOMEM); > + if (WARN_ON_ONCE(IS_ERR(old))) > return PTR_ERR(old); > - } > + > + if (tp_funcs == old) > + /* Failed allocating new tp_funcs, replaced func with stub */ > + return 0; > > if (!tp_funcs) { > /* Removed last function */ > -- > 2.25.4 >
