* [for-next][PATCH 14/15] tracepoint: Do not fail unregistering a probe due to memory failure
[not found] <20210203160517.982448432@goodmis.org>
@ 2021-02-03 16:05 ` Steven Rostedt
2021-02-03 17:05 ` Peter Zijlstra
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Steven Rostedt @ 2021-02-03 16:05 UTC (permalink / raw)
To: linux-kernel
Cc: Ingo Molnar, Andrew Morton, Peter Zijlstra, Josh Poimboeuf,
Mathieu Desnoyers, Ingo Molnar, Alexei Starovoitov,
Daniel Borkmann, Dmitry Vyukov, Martin KaFai Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh, netdev,
bpf, Kees Cook, Florian Weimer, syzbot+83aa762ef23b6f0d1991,
syzbot+d29e58bb557324e55e5e, Matt Mullins
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 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@mmlx.us
Link: https://lore.kernel.org/r/20201116175107.02db396d@gandalf.local.home
Link: https://lore.kernel.org/r/20201117211836.54acaef2@oasis.local.home
Link: https://lkml.kernel.org/r/20201118093405.7a6d2290@gandalf.local.home
[ Note, this version does use undefined compiler behavior (assuming that
a stub function with no parameters or return, can be called by a location
that thinks it has parameters but still no return value. Static calls
do the same thing, so this trick is not without precedent.
There's another solution that uses RCU tricks and is more complex, but
can be an alternative if this solution becomes an issue.
Link: https://lore.kernel.org/lkml/20210127170721.58bce7cc@gandalf.local.home/
]
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
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 <kafai@fb.com>
Cc: Song Liu <songliubraving@fb.com>
Cc: Yonghong Song <yhs@fb.com>
Cc: Andrii Nakryiko <andriin@fb.com>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: KP Singh <kpsingh@chromium.org>
Cc: netdev <netdev@vger.kernel.org>
Cc: bpf <bpf@vger.kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Florian Weimer <fw@deneb.enyo.de>
Fixes: 97e1c18e8d17b ("tracing: Kernel Tracepoints")
Reported-by: syzbot+83aa762ef23b6f0d1991@syzkaller.appspotmail.com
Reported-by: syzbot+d29e58bb557324e55e5e@syzkaller.appspotmail.com
Reported-by: Matt Mullins <mmullins@mmlx.us>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Tested-by: Matt Mullins <mmullins@mmlx.us>
---
kernel/tracepoint.c | 80 ++++++++++++++++++++++++++++++++++++---------
1 file changed, 64 insertions(+), 16 deletions(-)
diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
index 7261fa0f5e3c..e8f20ae29c18 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.29.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [for-next][PATCH 14/15] tracepoint: Do not fail unregistering a probe due to memory failure
2021-02-03 16:05 ` [for-next][PATCH 14/15] tracepoint: Do not fail unregistering a probe due to memory failure Steven Rostedt
@ 2021-02-03 17:05 ` Peter Zijlstra
2021-02-03 17:09 ` Peter Zijlstra
2021-02-03 17:57 ` Mathieu Desnoyers
2 siblings, 0 replies; 6+ messages in thread
From: Peter Zijlstra @ 2021-02-03 17:05 UTC (permalink / raw)
To: Steven Rostedt
Cc: linux-kernel, Ingo Molnar, Andrew Morton, Josh Poimboeuf,
Mathieu Desnoyers, Ingo Molnar, Alexei Starovoitov,
Daniel Borkmann, Dmitry Vyukov, Martin KaFai Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh, netdev,
bpf, Kees Cook, Florian Weimer, syzbot+83aa762ef23b6f0d1991,
syzbot+d29e58bb557324e55e5e, Matt Mullins
On Wed, Feb 03, 2021 at 11:05:31AM -0500, Steven Rostedt wrote:
> [ Note, this version does use undefined compiler behavior (assuming that
> a stub function with no parameters or return, can be called by a location
> that thinks it has parameters but still no return value. Static calls
> do the same thing, so this trick is not without precedent.
Specifically it relies on the C ABI being caller cleanup. CDECL is that.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [for-next][PATCH 14/15] tracepoint: Do not fail unregistering a probe due to memory failure
2021-02-03 16:05 ` [for-next][PATCH 14/15] tracepoint: Do not fail unregistering a probe due to memory failure Steven Rostedt
2021-02-03 17:05 ` Peter Zijlstra
@ 2021-02-03 17:09 ` Peter Zijlstra
2021-02-03 17:23 ` Steven Rostedt
2021-02-03 17:57 ` Mathieu Desnoyers
2 siblings, 1 reply; 6+ messages in thread
From: Peter Zijlstra @ 2021-02-03 17:09 UTC (permalink / raw)
To: Steven Rostedt
Cc: linux-kernel, Ingo Molnar, Andrew Morton, Josh Poimboeuf,
Mathieu Desnoyers, Ingo Molnar, Alexei Starovoitov,
Daniel Borkmann, Dmitry Vyukov, Martin KaFai Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh, netdev,
bpf, Kees Cook, Florian Weimer, syzbot+83aa762ef23b6f0d1991,
syzbot+d29e58bb557324e55e5e, Matt Mullins
On Wed, Feb 03, 2021 at 11:05:31AM -0500, Steven Rostedt wrote:
> + 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)
logical operators go at the end..
> + 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) {
like here.
> + 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;
multi line demands { }, but in this case just don't line-break.
> + 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;
{ }
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [for-next][PATCH 14/15] tracepoint: Do not fail unregistering a probe due to memory failure
2021-02-03 17:09 ` Peter Zijlstra
@ 2021-02-03 17:23 ` Steven Rostedt
0 siblings, 0 replies; 6+ messages in thread
From: Steven Rostedt @ 2021-02-03 17:23 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, Ingo Molnar, Andrew Morton, Josh Poimboeuf,
Mathieu Desnoyers, Ingo Molnar, Alexei Starovoitov,
Daniel Borkmann, Dmitry Vyukov, Martin KaFai Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh, netdev,
bpf, Kees Cook, Florian Weimer, syzbot+83aa762ef23b6f0d1991,
syzbot+d29e58bb557324e55e5e, Matt Mullins
On Wed, 3 Feb 2021 18:09:27 +0100
Peter Zijlstra <peterz@infradead.org> wrote:
> On Wed, Feb 03, 2021 at 11:05:31AM -0500, Steven Rostedt wrote:
> > + 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)
>
> logical operators go at the end..
Agreed. I just added that "if (new) {" around the original block, didn't
think about the formatting when doing so.
>
> > + 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) {
>
> like here.
>
> > + 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;
>
> multi line demands { }, but in this case just don't line-break.
Sure.
>
> > + 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;
>
> { }
Even if it's just a comment that causes multiple lines? I could just move
the comment above the if.
This has already been through my test suite, and since the changes
requested are just formatting and non-functional, I'll just add a clean up
patch on top.
Thanks!
-- Steve
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [for-next][PATCH 14/15] tracepoint: Do not fail unregistering a probe due to memory failure
2021-02-03 16:05 ` [for-next][PATCH 14/15] tracepoint: Do not fail unregistering a probe due to memory failure Steven Rostedt
2021-02-03 17:05 ` Peter Zijlstra
2021-02-03 17:09 ` Peter Zijlstra
@ 2021-02-03 17:57 ` Mathieu Desnoyers
2021-02-04 16:50 ` Steven Rostedt
2 siblings, 1 reply; 6+ messages in thread
From: Mathieu Desnoyers @ 2021-02-03 17:57 UTC (permalink / raw)
To: rostedt
Cc: linux-kernel, Ingo Molnar, Andrew Morton, Peter Zijlstra,
Josh Poimboeuf, Ingo Molnar, Alexei Starovoitov, Daniel Borkmann,
Dmitry Vyukov, Martin KaFai Lau, Song Liu, Yonghong Song,
Andrii Nakryiko, John Fastabend, KP Singh, netdev, bpf,
Kees Cook, Florian Weimer, syzbot+83aa762ef23b6f0d1991,
syzbot+d29e58bb557324e55e5e, Matt Mullins
----- On Feb 3, 2021, at 11:05 AM, rostedt rostedt@goodmis.org wrote:
> 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 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@mmlx.us
> Link: https://lore.kernel.org/r/20201116175107.02db396d@gandalf.local.home
> Link: https://lore.kernel.org/r/20201117211836.54acaef2@oasis.local.home
> Link: https://lkml.kernel.org/r/20201118093405.7a6d2290@gandalf.local.home
>
> [ Note, this version does use undefined compiler behavior (assuming that
> a stub function with no parameters or return, can be called by a location
> that thinks it has parameters but still no return value. Static calls
> do the same thing, so this trick is not without precedent.
>
> There's another solution that uses RCU tricks and is more complex, but
> can be an alternative if this solution becomes an issue.
>
> Link: https://lore.kernel.org/lkml/20210127170721.58bce7cc@gandalf.local.home/
> ]
>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> 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 <kafai@fb.com>
> Cc: Song Liu <songliubraving@fb.com>
> Cc: Yonghong Song <yhs@fb.com>
> Cc: Andrii Nakryiko <andriin@fb.com>
> Cc: John Fastabend <john.fastabend@gmail.com>
> Cc: KP Singh <kpsingh@chromium.org>
> Cc: netdev <netdev@vger.kernel.org>
> Cc: bpf <bpf@vger.kernel.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Florian Weimer <fw@deneb.enyo.de>
> Fixes: 97e1c18e8d17b ("tracing: Kernel Tracepoints")
> Reported-by: syzbot+83aa762ef23b6f0d1991@syzkaller.appspotmail.com
> Reported-by: syzbot+d29e58bb557324e55e5e@syzkaller.appspotmail.com
> Reported-by: Matt Mullins <mmullins@mmlx.us>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> Tested-by: Matt Mullins <mmullins@mmlx.us>
> ---
> kernel/tracepoint.c | 80 ++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 64 insertions(+), 16 deletions(-)
>
> diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
> index 7261fa0f5e3c..e8f20ae29c18 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) {
Considering that we end up implementing a case where we carefully copy over
each item, I recommend we replace the two "memcpy" branches by a single item-wise
implementation. It's a slow-path anyway, and reducing the overall complexity
is a benefit for slow paths. Fewer bugs, less code to review, and it's easier to
reach a decent testing state-space coverage.
> + /* 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;
Repurposing "nr_probes" from accounting for the number of items in the old
array to counting the number of items in the new array in the middle of the
function is confusing.
> + if (pos < 0)
> + pos = probes;
> + else
> + nr_probes--; /* Account for insertion */
This is probably why you need to play tricks with nr_probes here.
> + } 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;
This updates "func" while readers are loading it concurrently. I would recommend
using WRITE_ONCE here paired with READ_ONCE within __traceiter_##_name.
> + /* Set the prio to the next event. */
I don't get why the priority needs to be changed here. Could it simply stay
at its original value ? It's already in the correct priority order anyway.
> + if (old[i + 1].func)
> + old[i].prio =
> + old[i + 1].prio;
> + else
> + old[i].prio = -1;
> + }
> + *funcs = old;
I'm not sure what setting *funcs to old achieves ? Isn't it already pointing
to old ?
I'll send a patch which applies on top of yours implementing my recommendations.
It shrinks the code complexity nicely:
include/linux/tracepoint.h | 2 +-
kernel/tracepoint.c | 80 +++++++++++++-------------------------
2 files changed, 28 insertions(+), 54 deletions(-)
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [for-next][PATCH 14/15] tracepoint: Do not fail unregistering a probe due to memory failure
2021-02-03 17:57 ` Mathieu Desnoyers
@ 2021-02-04 16:50 ` Steven Rostedt
0 siblings, 0 replies; 6+ messages in thread
From: Steven Rostedt @ 2021-02-04 16:50 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: linux-kernel, Ingo Molnar, Andrew Morton, Peter Zijlstra,
Josh Poimboeuf, Ingo Molnar, Alexei Starovoitov, Daniel Borkmann,
Dmitry Vyukov, Martin KaFai Lau, Song Liu, Yonghong Song,
Andrii Nakryiko, John Fastabend, KP Singh, netdev, bpf,
Kees Cook, Florian Weimer, syzbot+83aa762ef23b6f0d1991,
syzbot+d29e58bb557324e55e5e, Matt Mullins
On Wed, 3 Feb 2021 12:57:24 -0500 (EST)
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> > @@ -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) {
>
> Considering that we end up implementing a case where we carefully copy over
> each item, I recommend we replace the two "memcpy" branches by a single item-wise
> implementation. It's a slow-path anyway, and reducing the overall complexity
> is a benefit for slow paths. Fewer bugs, less code to review, and it's easier to
> reach a decent testing state-space coverage.
Sure.
>
> > + /* 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;
>
> Repurposing "nr_probes" from accounting for the number of items in the old
> array to counting the number of items in the new array in the middle of the
> function is confusing.
>
> > + if (pos < 0)
> > + pos = probes;
> > + else
> > + nr_probes--; /* Account for insertion */
>
> This is probably why you need to play tricks with nr_probes here.
>
> > + } 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;
>
> This updates "func" while readers are loading it concurrently. I would recommend
> using WRITE_ONCE here paired with READ_ONCE within __traceiter_##_name.
I'm fine with this change, but it shouldn't make a difference. As we don't
change the data, it doesn't matter which function the compiler calls.
Unless you are worried about the compiler tearing the read. It shouldn't,
but I'm fine with doing things for paranoid sake (especially if it doesn't
affect the performance).
>
> > + /* Set the prio to the next event. */
>
> I don't get why the priority needs to be changed here. Could it simply stay
> at its original value ? It's already in the correct priority order anyway.
I think it was left over from one of the various changes. As I went to v5,
and then back to v3, I missed revisiting the code, as I was under the
assumption that I had cleaned it up :-/
>
> > + if (old[i + 1].func)
> > + old[i].prio =
> > + old[i + 1].prio;
> > + else
> > + old[i].prio = -1;
> > + }
> > + *funcs = old;
>
> I'm not sure what setting *funcs to old achieves ? Isn't it already pointing
> to old ?
Again, I think one iteration may have changed it. And I kinda remember
keeping it just to be consistent (*funcs gets updated in the other paths,
and figured it was good to be "safe" and updated it again, even though the
logic has it already set).
>
> I'll send a patch which applies on top of yours implementing my recommendations.
> It shrinks the code complexity nicely:
Looking at it now.
Thanks,
-- Steve
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-02-04 16:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20210203160517.982448432@goodmis.org>
2021-02-03 16:05 ` [for-next][PATCH 14/15] tracepoint: Do not fail unregistering a probe due to memory failure Steven Rostedt
2021-02-03 17:05 ` Peter Zijlstra
2021-02-03 17:09 ` Peter Zijlstra
2021-02-03 17:23 ` Steven Rostedt
2021-02-03 17:57 ` Mathieu Desnoyers
2021-02-04 16:50 ` Steven Rostedt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).