While auditing all module notifiers I noticed a whole bunch of fail
wrt the return value. Notifiers have a 'special' return semantics.
Cc: Robert Richter <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Alexei Starovoitov <[email protected]>
Cc: Daniel Borkmann <[email protected]>
Cc: Martin KaFai Lau <[email protected]>
Cc: Song Liu <[email protected]>
Cc: Yonghong Song <[email protected]>
Cc: Mathieu Desnoyers <[email protected]>
Cc: "Paul E. McKenney" <[email protected]>
Cc: "Joel Fernandes (Google)" <[email protected]>
Cc: Ard Biesheuvel <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
---
drivers/oprofile/buffer_sync.c | 4 ++--
kernel/module.c | 9 +++++----
kernel/trace/bpf_trace.c | 8 ++++++--
kernel/trace/trace.c | 2 +-
kernel/trace/trace_events.c | 2 +-
kernel/trace/trace_printk.c | 4 ++--
kernel/tracepoint.c | 2 +-
7 files changed, 18 insertions(+), 13 deletions(-)
--- a/drivers/oprofile/buffer_sync.c
+++ b/drivers/oprofile/buffer_sync.c
@@ -116,7 +116,7 @@ module_load_notify(struct notifier_block
{
#ifdef CONFIG_MODULES
if (val != MODULE_STATE_COMING)
- return 0;
+ return NOTIFY_DONE;
/* FIXME: should we process all CPU buffers ? */
mutex_lock(&buffer_mutex);
@@ -124,7 +124,7 @@ module_load_notify(struct notifier_block
add_event_entry(MODULE_LOADED_CODE);
mutex_unlock(&buffer_mutex);
#endif
- return 0;
+ return NOTIFY_OK;
}
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1302,10 +1302,11 @@ static int bpf_event_notify(struct notif
{
struct bpf_trace_module *btm, *tmp;
struct module *mod = module;
+ int ret = 0;
if (mod->num_bpf_raw_events == 0 ||
(op != MODULE_STATE_COMING && op != MODULE_STATE_GOING))
- return 0;
+ goto out;
mutex_lock(&bpf_module_mutex);
@@ -1315,6 +1316,8 @@ static int bpf_event_notify(struct notif
if (btm) {
btm->module = module;
list_add(&btm->list, &bpf_trace_modules);
+ } else {
+ ret = -ENOMEM;
}
break;
case MODULE_STATE_GOING:
@@ -1330,7 +1333,8 @@ static int bpf_event_notify(struct notif
mutex_unlock(&bpf_module_mutex);
- return 0;
+out:
+ return notifier_from_errno(ret);
}
static struct notifier_block bpf_module_nb = {
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -8685,7 +8685,7 @@ static int trace_module_notify(struct no
break;
}
- return 0;
+ return NOTIFY_OK;
}
static struct notifier_block trace_module_nb = {
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -2450,7 +2450,7 @@ static int trace_module_notify(struct no
mutex_unlock(&trace_types_lock);
mutex_unlock(&event_mutex);
- return 0;
+ return NOTIFY_OK;
}
static struct notifier_block trace_module_nb = {
--- a/kernel/trace/trace_printk.c
+++ b/kernel/trace/trace_printk.c
@@ -95,7 +95,7 @@ static int module_trace_bprintk_format_n
if (val == MODULE_STATE_COMING)
hold_module_trace_bprintk_format(start, end);
}
- return 0;
+ return NOTIFY_OK;
}
/*
@@ -173,7 +173,7 @@ __init static int
module_trace_bprintk_format_notify(struct notifier_block *self,
unsigned long val, void *data)
{
- return 0;
+ return NOTIFY_OK;
}
static inline const char **
find_next_mod_format(int start_index, void *v, const char **fmt, loff_t *pos)
--- a/kernel/tracepoint.c
+++ b/kernel/tracepoint.c
@@ -538,7 +538,7 @@ static int tracepoint_module_notify(stru
case MODULE_STATE_UNFORMED:
break;
}
- return ret;
+ return notifier_from_errno(ret);
}
static struct notifier_block tracepoint_module_nb = {
----- On Jun 24, 2019, at 5:18 AM, Peter Zijlstra [email protected] wrote:
> While auditing all module notifiers I noticed a whole bunch of fail
> wrt the return value. Notifiers have a 'special' return semantics.
>
> Cc: Robert Richter <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Alexei Starovoitov <[email protected]>
> Cc: Daniel Borkmann <[email protected]>
> Cc: Martin KaFai Lau <[email protected]>
> Cc: Song Liu <[email protected]>
> Cc: Yonghong Song <[email protected]>
> Cc: Mathieu Desnoyers <[email protected]>
> Cc: "Paul E. McKenney" <[email protected]>
> Cc: "Joel Fernandes (Google)" <[email protected]>
> Cc: Ard Biesheuvel <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Thanks Peter for looking into this, especially considering your
endless love for kernel modules! ;)
It's not directly related to your changes, but I notice that
kernel/trace/trace_printk.c:hold_module_trace_bprintk_format()
appears to leak memory. Am I missing something ?
With respect to your changes:
Reviewed-by: Mathieu Desnoyers <[email protected]>
I have a similar erroneous module notifier return value pattern
in lttng-modules as well. I'll go fix it right away. CCing
Frank Eigler from SystemTAP which AFAIK use a copy of
lttng-tracepoint.c in their project, which should be fixed
as well. I'm pasting the lttng-modules fix below.
Thanks!
Mathieu
--
commit 5eac9d146a7d947f0f314c4f7103c92cbccaeaf3
Author: Mathieu Desnoyers <[email protected]>
Date: Mon Jun 24 09:43:45 2019 -0400
Fix: lttng-tracepoint module notifier should return NOTIFY_OK
Module notifiers should return NOTIFY_OK on success rather than the
value 0. The return value 0 does not seem to have any ill side-effects
in the notifier chain caller, but it is preferable to respect the API
requirements in case this changes in the future.
Notifiers can encapsulate a negative errno value with
notifier_from_errno(), but this is not needed by the LTTng tracepoint
notifier.
The approach taken in this notifier is to just print a console warning
on error, because tracing failure should not prevent loading a module.
So we definitely do not want to stop notifier iteration. Returning
an error without stopping iteration is not really that useful, because
only the return value of the last callback is returned to notifier chain
caller.
Signed-off-by: Mathieu Desnoyers <[email protected]>
diff --git a/lttng-tracepoint.c b/lttng-tracepoint.c
index bbb2c7a4..8298b397 100644
--- a/lttng-tracepoint.c
+++ b/lttng-tracepoint.c
@@ -256,7 +256,7 @@ int lttng_tracepoint_coming(struct tp_module *tp_mod)
}
}
mutex_unlock(<tng_tracepoint_mutex);
- return 0;
+ return NOTIFY_OK;
}
static
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
On Mon, Jun 24, 2019 at 10:01:04AM -0400, Mathieu Desnoyers wrote:
> ----- On Jun 24, 2019, at 5:18 AM, Peter Zijlstra [email protected] wrote:
>
> > While auditing all module notifiers I noticed a whole bunch of fail
> > wrt the return value. Notifiers have a 'special' return semantics.
> >
> > Cc: Robert Richter <[email protected]>
> > Cc: Steven Rostedt <[email protected]>
> > Cc: Ingo Molnar <[email protected]>
> > Cc: Alexei Starovoitov <[email protected]>
> > Cc: Daniel Borkmann <[email protected]>
> > Cc: Martin KaFai Lau <[email protected]>
> > Cc: Song Liu <[email protected]>
> > Cc: Yonghong Song <[email protected]>
> > Cc: Mathieu Desnoyers <[email protected]>
> > Cc: "Paul E. McKenney" <[email protected]>
> > Cc: "Joel Fernandes (Google)" <[email protected]>
> > Cc: Ard Biesheuvel <[email protected]>
> > Cc: Thomas Gleixner <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Cc: [email protected]
> > Cc: [email protected]
> > Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
>
> Thanks Peter for looking into this, especially considering your
> endless love for kernel modules! ;)
>
> It's not directly related to your changes, but I notice that
> kernel/trace/trace_printk.c:hold_module_trace_bprintk_format()
> appears to leak memory. Am I missing something ?
Could you elaborate? Do you mean there is no MODULE_STATE_GOING notifier
check? If that's what you mean then I agree, there should be some place
where the format structures are freed when the module is unloaded no?
>
> With respect to your changes:
> Reviewed-by: Mathieu Desnoyers <[email protected]>
Looks good to me too.
Reviewed-by: Joel Fernandes (Google) <[email protected]>
Could we CC stable so that the fix is propagated to older kernels?
thanks,
- Joel
> I have a similar erroneous module notifier return value pattern
> in lttng-modules as well. I'll go fix it right away. CCing
> Frank Eigler from SystemTAP which AFAIK use a copy of
> lttng-tracepoint.c in their project, which should be fixed
> as well. I'm pasting the lttng-modules fix below.
>
> Thanks!
>
> Mathieu
>
> --
>
> commit 5eac9d146a7d947f0f314c4f7103c92cbccaeaf3
> Author: Mathieu Desnoyers <[email protected]>
> Date: Mon Jun 24 09:43:45 2019 -0400
>
> Fix: lttng-tracepoint module notifier should return NOTIFY_OK
>
> Module notifiers should return NOTIFY_OK on success rather than the
> value 0. The return value 0 does not seem to have any ill side-effects
> in the notifier chain caller, but it is preferable to respect the API
> requirements in case this changes in the future.
>
> Notifiers can encapsulate a negative errno value with
> notifier_from_errno(), but this is not needed by the LTTng tracepoint
> notifier.
>
> The approach taken in this notifier is to just print a console warning
> on error, because tracing failure should not prevent loading a module.
> So we definitely do not want to stop notifier iteration. Returning
> an error without stopping iteration is not really that useful, because
> only the return value of the last callback is returned to notifier chain
> caller.
>
> Signed-off-by: Mathieu Desnoyers <[email protected]>
>
> diff --git a/lttng-tracepoint.c b/lttng-tracepoint.c
> index bbb2c7a4..8298b397 100644
> --- a/lttng-tracepoint.c
> +++ b/lttng-tracepoint.c
> @@ -256,7 +256,7 @@ int lttng_tracepoint_coming(struct tp_module *tp_mod)
> }
> }
> mutex_unlock(<tng_tracepoint_mutex);
> - return 0;
> + return NOTIFY_OK;
> }
>
> static
>
>
> --
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com
----- On Jun 24, 2019, at 11:52 AM, Joel Fernandes, Google [email protected] wrote:
> On Mon, Jun 24, 2019 at 10:01:04AM -0400, Mathieu Desnoyers wrote:
>> ----- On Jun 24, 2019, at 5:18 AM, Peter Zijlstra [email protected] wrote:
>>
>> > While auditing all module notifiers I noticed a whole bunch of fail
>> > wrt the return value. Notifiers have a 'special' return semantics.
>> >
>> > Cc: Robert Richter <[email protected]>
>> > Cc: Steven Rostedt <[email protected]>
>> > Cc: Ingo Molnar <[email protected]>
>> > Cc: Alexei Starovoitov <[email protected]>
>> > Cc: Daniel Borkmann <[email protected]>
>> > Cc: Martin KaFai Lau <[email protected]>
>> > Cc: Song Liu <[email protected]>
>> > Cc: Yonghong Song <[email protected]>
>> > Cc: Mathieu Desnoyers <[email protected]>
>> > Cc: "Paul E. McKenney" <[email protected]>
>> > Cc: "Joel Fernandes (Google)" <[email protected]>
>> > Cc: Ard Biesheuvel <[email protected]>
>> > Cc: Thomas Gleixner <[email protected]>
>> > Cc: [email protected]
>> > Cc: [email protected]
>> > Cc: [email protected]
>> > Cc: [email protected]
>> > Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
>>
>> Thanks Peter for looking into this, especially considering your
>> endless love for kernel modules! ;)
>>
>> It's not directly related to your changes, but I notice that
>> kernel/trace/trace_printk.c:hold_module_trace_bprintk_format()
>> appears to leak memory. Am I missing something ?
>
> Could you elaborate? Do you mean there is no MODULE_STATE_GOING notifier
> check? If that's what you mean then I agree, there should be some place
> where the format structures are freed when the module is unloaded no?
Yes, the lack of GOING notifier is worrying considering that GOING
performs memory allocation.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
Hi -
> > While auditing all module notifiers I noticed a whole bunch of fail
> > wrt the return value. Notifiers have a 'special' return semantics.
From peterz's comments, the patches, it's not obvious to me how one is
to choose between 0 (NOTIFY_DONE) and 1 (NOTIFY_OK) in the case of a
routine success.
> [...]
> I have a similar erroneous module notifier return value pattern
> in lttng-modules as well. I'll go fix it right away. CCing
> Frank Eigler from SystemTAP which AFAIK use a copy of
> lttng-tracepoint.c in their project, which should be fixed
> as well. I'm pasting the lttng-modules fix below.
Sure, following suit. Thanks.
- FChE
On Mon, Jun 24, 2019 at 04:58:10PM -0400, Frank Ch. Eigler wrote:
> Hi -
>
> > > While auditing all module notifiers I noticed a whole bunch of fail
> > > wrt the return value. Notifiers have a 'special' return semantics.
>
> From peterz's comments, the patches, it's not obvious to me how one is
> to choose between 0 (NOTIFY_DONE) and 1 (NOTIFY_OK) in the case of a
> routine success.
I'm not sure either; what I think I choice was:
- if I want to completely ignore the callback, use DONE (per the
"Don't care" comment).
- if we finished the notifier without error, use OK or
notifier_from_errno(0).
But yes, its a bit of a shit interface.
On 24.06.19 11:18:45, Peter Zijlstra wrote:
> While auditing all module notifiers I noticed a whole bunch of fail
> wrt the return value. Notifiers have a 'special' return semantics.
>
> Cc: Robert Richter <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Alexei Starovoitov <[email protected]>
> Cc: Daniel Borkmann <[email protected]>
> Cc: Martin KaFai Lau <[email protected]>
> Cc: Song Liu <[email protected]>
> Cc: Yonghong Song <[email protected]>
> Cc: Mathieu Desnoyers <[email protected]>
> Cc: "Paul E. McKenney" <[email protected]>
> Cc: "Joel Fernandes (Google)" <[email protected]>
> Cc: Ard Biesheuvel <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> ---
> drivers/oprofile/buffer_sync.c | 4 ++--
> kernel/module.c | 9 +++++----
> kernel/trace/bpf_trace.c | 8 ++++++--
> kernel/trace/trace.c | 2 +-
> kernel/trace/trace_events.c | 2 +-
> kernel/trace/trace_printk.c | 4 ++--
> kernel/tracepoint.c | 2 +-
> 7 files changed, 18 insertions(+), 13 deletions(-)
Reviewed-by: Robert Richter <[email protected]>
On 25.06.19 09:42:14, Peter Zijlstra wrote:
> On Mon, Jun 24, 2019 at 04:58:10PM -0400, Frank Ch. Eigler wrote:
> > From peterz's comments, the patches, it's not obvious to me how one is
> > to choose between 0 (NOTIFY_DONE) and 1 (NOTIFY_OK) in the case of a
> > routine success.
>
> I'm not sure either; what I think I choice was:
>
> - if I want to completely ignore the callback, use DONE (per the
> "Don't care" comment).
>
> - if we finished the notifier without error, use OK or
> notifier_from_errno(0).
>
> But yes, its a bit of a shit interface.
It looks like it was rarely used in earlier kernels as some sort of
error detection for the notifier calls, e.g.:
1da177e4c3f41524e886b7f1b8a0c1fc7321cac2:kernel/profile.c-int profile_handoff_task(struct task_struct * task)
1da177e4c3f41524e886b7f1b8a0c1fc7321cac2:kernel/profile.c-{
1da177e4c3f41524e886b7f1b8a0c1fc7321cac2:kernel/profile.c- int ret;
1da177e4c3f41524e886b7f1b8a0c1fc7321cac2:kernel/profile.c- read_lock(&handoff_lock);
1da177e4c3f41524e886b7f1b8a0c1fc7321cac2:kernel/profile.c- ret = notifier_call_chain(&task_free_notifier, 0, task);
1da177e4c3f41524e886b7f1b8a0c1fc7321cac2:kernel/profile.c- read_unlock(&handoff_lock);
1da177e4c3f41524e886b7f1b8a0c1fc7321cac2:kernel/profile.c: return (ret == NOTIFY_OK) ? 1 : 0;
1da177e4c3f41524e886b7f1b8a0c1fc7321cac2:kernel/profile.c-}
So NOTIFY_OK was used to state there is no error, while NOTIFY_DONE
says the notifier was executed and there might have been errors. The
caller may distinguish the results then.
-Robert