On Wed, Nov 18, 2020 at 09:34:05AM -0500, Steven Rostedt wrote:
> From: "Steven Rostedt (VMware)" <rost...@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 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-mmull...@mmlx.us
> Link: https://lore.kernel.org/r/20201116175107.02db3...@gandalf.local.home
> Link: https://lore.kernel.org/r/20201117211836.54aca...@oasis.local.home
>
> Cc: Mathieu Desnoyers <mathieu.desnoy...@efficios.com>
> Cc: Ingo Molnar <mi...@redhat.com>
> Cc: Alexei Starovoitov <a...@kernel.org>
> Cc: Daniel Borkmann <dan...@iogearbox.net>
> Cc: Dmitry Vyukov <dvyu...@google.com>
> Cc: Martin KaFai Lau <ka...@fb.com>
> Cc: Song Liu <songliubrav...@fb.com>
> Cc: Yonghong Song <y...@fb.com>
> Cc: Andrii Nakryiko <andr...@fb.com>
> Cc: John Fastabend <john.fastab...@gmail.com>
> Cc: KP Singh <kpsi...@chromium.org>
> Cc: netdev <net...@vger.kernel.org>
> Cc: bpf <b...@vger.kernel.org>
> Cc: Kees Cook <keesc...@chromium.org>
> Cc: Florian Weimer <f...@deneb.enyo.de>
> Fixes: 97e1c18e8d17b ("tracing: Kernel Tracepoints")
> Reported-by: syzbot+83aa762ef23b6f0d1...@syzkaller.appspotmail.com
> Reported-by: syzbot+d29e58bb557324e55...@syzkaller.appspotmail.com
> Reported-by: Matt Mullins <mmull...@mmlx.us>
> Signed-off-by: Steven Rostedt (VMware) <rost...@goodmis.org>
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 <mmull...@mmlx.us>
> ---
> 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
>
