netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 2/3] module: Fix up module_notifier return values.
       [not found] <20190624091843.859714294@infradead.org>
@ 2019-06-24  9:18 ` Peter Zijlstra
  2019-06-24 14:01   ` Mathieu Desnoyers
  2019-07-04 12:34   ` Robert Richter
  2019-06-24  9:18 ` [PATCH 3/3] module: Properly propagate MODULE_STATE_COMING failure Peter Zijlstra
  1 sibling, 2 replies; 10+ messages in thread
From: Peter Zijlstra @ 2019-06-24  9:18 UTC (permalink / raw)
  To: Jessica Yu, linux-kernel, jpoimboe, jikos, mbenes, pmladek, ast,
	daniel, akpm, peterz
  Cc: Robert Richter, Steven Rostedt, Ingo Molnar, Martin KaFai Lau,
	Song Liu, Yonghong Song, Mathieu Desnoyers, Paul E. McKenney,
	Joel Fernandes (Google),
	Ard Biesheuvel, Thomas Gleixner, oprofile-list, netdev, bpf

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 <rric@kernel.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Martin KaFai Lau <kafai@fb.com>
Cc: Song Liu <songliubraving@fb.com>
Cc: Yonghong Song <yhs@fb.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: "Paul E. McKenney" <paulmck@linux.ibm.com>
Cc: "Joel Fernandes (Google)" <joel@joelfernandes.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: oprofile-list@lists.sf.net
Cc: linux-kernel@vger.kernel.org
Cc: netdev@vger.kernel.org
Cc: bpf@vger.kernel.org
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 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 = {



^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 3/3] module: Properly propagate MODULE_STATE_COMING failure
       [not found] <20190624091843.859714294@infradead.org>
  2019-06-24  9:18 ` [PATCH 2/3] module: Fix up module_notifier return values Peter Zijlstra
@ 2019-06-24  9:18 ` Peter Zijlstra
  2019-06-24 11:07   ` Peter Zijlstra
  1 sibling, 1 reply; 10+ messages in thread
From: Peter Zijlstra @ 2019-06-24  9:18 UTC (permalink / raw)
  To: Jessica Yu, linux-kernel, jpoimboe, jikos, mbenes, pmladek, ast,
	daniel, akpm, peterz
  Cc: Martin KaFai Lau, Song Liu, Yonghong Song, netdev, bpf

Now that notifiers got unbroken; use the proper interface to handle
notifier errors and propagate them.

There were already MODULE_STATE_COMING notifiers that failed; notably:

 - jump_label_module_notifier()
 - tracepoint_module_notify()
 - bpf_event_notify()

By propagating this error, we fix those users.

Cc: Jessica Yu <jeyu@kernel.org>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Martin KaFai Lau <kafai@fb.com>
Cc: Song Liu <songliubraving@fb.com>
Cc: Yonghong Song <yhs@fb.com>
Cc: linux-kernel@vger.kernel.org
Cc: netdev@vger.kernel.org
Cc: bpf@vger.kernel.org
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/module.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3643,9 +3643,10 @@ static int prepare_coming_module(struct
 	if (err)
 		return err;
 
-	blocking_notifier_call_chain(&module_notify_list,
-				     MODULE_STATE_COMING, mod);
-	return 0;
+	err = blocking_notifier_call_chain_error(&module_notify_list,
+			MODULE_STATE_COMING, MODULE_STATE_GOING, mod);
+
+	return notifier_to_errno(err);
 }
 
 static int unknown_module_param_cb(char *param, char *val, const char *modname,



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 3/3] module: Properly propagate MODULE_STATE_COMING failure
  2019-06-24  9:18 ` [PATCH 3/3] module: Properly propagate MODULE_STATE_COMING failure Peter Zijlstra
@ 2019-06-24 11:07   ` Peter Zijlstra
  0 siblings, 0 replies; 10+ messages in thread
From: Peter Zijlstra @ 2019-06-24 11:07 UTC (permalink / raw)
  To: Jessica Yu, linux-kernel, jpoimboe, jikos, mbenes, pmladek, ast,
	daniel, akpm
  Cc: Martin KaFai Lau, Song Liu, Yonghong Song, netdev, bpf

On Mon, Jun 24, 2019 at 11:18:46AM +0200, Peter Zijlstra wrote:
> Now that notifiers got unbroken; use the proper interface to handle
> notifier errors and propagate them.
> 
> There were already MODULE_STATE_COMING notifiers that failed; notably:
> 
>  - jump_label_module_notifier()
>  - tracepoint_module_notify()
>  - bpf_event_notify()
> 
> By propagating this error, we fix those users.
> 
> Cc: Jessica Yu <jeyu@kernel.org>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Martin KaFai Lau <kafai@fb.com>
> Cc: Song Liu <songliubraving@fb.com>
> Cc: Yonghong Song <yhs@fb.com>
> Cc: linux-kernel@vger.kernel.org
> Cc: netdev@vger.kernel.org
> Cc: bpf@vger.kernel.org
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  kernel/module.c |    7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3643,9 +3643,10 @@ static int prepare_coming_module(struct
>  	if (err)
>  		return err;
>  
> -	blocking_notifier_call_chain(&module_notify_list,
> -				     MODULE_STATE_COMING, mod);
> -	return 0;
> +	err = blocking_notifier_call_chain_error(&module_notify_list,
> +			MODULE_STATE_COMING, MODULE_STATE_GOING, mod);
> +
> +	return notifier_to_errno(err);
>  }

Argh, I messed up that klp thing again :/

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/3] module: Fix up module_notifier return values.
  2019-06-24  9:18 ` [PATCH 2/3] module: Fix up module_notifier return values Peter Zijlstra
@ 2019-06-24 14:01   ` Mathieu Desnoyers
  2019-06-24 15:52     ` Joel Fernandes
  2019-06-24 20:58     ` Frank Ch. Eigler
  2019-07-04 12:34   ` Robert Richter
  1 sibling, 2 replies; 10+ messages in thread
From: Mathieu Desnoyers @ 2019-06-24 14:01 UTC (permalink / raw)
  To: Peter Zijlstra, Frank Ch. Eigler
  Cc: Jessica Yu, linux-kernel, Josh Poimboeuf, jikos, mbenes,
	Petr Mladek, Alexei Starovoitov, Daniel Borkmann, Andrew Morton,
	Robert Richter, rostedt, Ingo Molnar, Martin KaFai Lau, Song Liu,
	Yonghong Song, paulmck, Joel Fernandes, Google, Ard Biesheuvel,
	Thomas Gleixner, oprofile-list, netdev, bpf

----- On Jun 24, 2019, at 5:18 AM, Peter Zijlstra peterz@infradead.org 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 <rric@kernel.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Martin KaFai Lau <kafai@fb.com>
> Cc: Song Liu <songliubraving@fb.com>
> Cc: Yonghong Song <yhs@fb.com>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: "Paul E. McKenney" <paulmck@linux.ibm.com>
> Cc: "Joel Fernandes (Google)" <joel@joelfernandes.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: oprofile-list@lists.sf.net
> Cc: linux-kernel@vger.kernel.org
> Cc: netdev@vger.kernel.org
> Cc: bpf@vger.kernel.org
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

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 <mathieu.desnoyers@efficios.com>

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 <mathieu.desnoyers@efficios.com>
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 <mathieu.desnoyers@efficios.com>

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(&lttng_tracepoint_mutex);
-       return 0;
+       return NOTIFY_OK;
 }
 
 static


-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/3] module: Fix up module_notifier return values.
  2019-06-24 14:01   ` Mathieu Desnoyers
@ 2019-06-24 15:52     ` Joel Fernandes
  2019-06-24 17:50       ` Mathieu Desnoyers
  2019-06-24 20:58     ` Frank Ch. Eigler
  1 sibling, 1 reply; 10+ messages in thread
From: Joel Fernandes @ 2019-06-24 15:52 UTC (permalink / raw)
  To: Mathieu Desnoyers
  Cc: Peter Zijlstra, Frank Ch. Eigler, Jessica Yu, linux-kernel,
	Josh Poimboeuf, jikos, mbenes, Petr Mladek, Alexei Starovoitov,
	Daniel Borkmann, Andrew Morton, Robert Richter, rostedt,
	Ingo Molnar, Martin KaFai Lau, Song Liu, Yonghong Song, paulmck,
	Ard Biesheuvel, Thomas Gleixner, oprofile-list, netdev, bpf

On Mon, Jun 24, 2019 at 10:01:04AM -0400, Mathieu Desnoyers wrote:
> ----- On Jun 24, 2019, at 5:18 AM, Peter Zijlstra peterz@infradead.org 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 <rric@kernel.org>
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: Alexei Starovoitov <ast@kernel.org>
> > Cc: Daniel Borkmann <daniel@iogearbox.net>
> > Cc: Martin KaFai Lau <kafai@fb.com>
> > Cc: Song Liu <songliubraving@fb.com>
> > Cc: Yonghong Song <yhs@fb.com>
> > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > Cc: "Paul E. McKenney" <paulmck@linux.ibm.com>
> > Cc: "Joel Fernandes (Google)" <joel@joelfernandes.org>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: oprofile-list@lists.sf.net
> > Cc: linux-kernel@vger.kernel.org
> > Cc: netdev@vger.kernel.org
> > Cc: bpf@vger.kernel.org
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> 
> 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 <mathieu.desnoyers@efficios.com>

Looks good to me too.

Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

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 <mathieu.desnoyers@efficios.com>
> 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 <mathieu.desnoyers@efficios.com>
> 
> 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(&lttng_tracepoint_mutex);
> -       return 0;
> +       return NOTIFY_OK;
>  }
>  
>  static
> 
> 
> -- 
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/3] module: Fix up module_notifier return values.
  2019-06-24 15:52     ` Joel Fernandes
@ 2019-06-24 17:50       ` Mathieu Desnoyers
  0 siblings, 0 replies; 10+ messages in thread
From: Mathieu Desnoyers @ 2019-06-24 17:50 UTC (permalink / raw)
  To: Joel Fernandes, Google
  Cc: Peter Zijlstra, Frank Ch. Eigler, Jessica Yu, linux-kernel,
	Josh Poimboeuf, jikos, mbenes, Petr Mladek, Alexei Starovoitov,
	Daniel Borkmann, Andrew Morton, Robert Richter, rostedt,
	Ingo Molnar, Martin KaFai Lau, Song Liu, Yonghong Song, paulmck,
	Ard Biesheuvel, Thomas Gleixner, oprofile-list, netdev, bpf

----- On Jun 24, 2019, at 11:52 AM, Joel Fernandes, Google joel@joelfernandes.org wrote:

> On Mon, Jun 24, 2019 at 10:01:04AM -0400, Mathieu Desnoyers wrote:
>> ----- On Jun 24, 2019, at 5:18 AM, Peter Zijlstra peterz@infradead.org 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 <rric@kernel.org>
>> > Cc: Steven Rostedt <rostedt@goodmis.org>
>> > Cc: Ingo Molnar <mingo@redhat.com>
>> > Cc: Alexei Starovoitov <ast@kernel.org>
>> > Cc: Daniel Borkmann <daniel@iogearbox.net>
>> > Cc: Martin KaFai Lau <kafai@fb.com>
>> > Cc: Song Liu <songliubraving@fb.com>
>> > Cc: Yonghong Song <yhs@fb.com>
>> > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>> > Cc: "Paul E. McKenney" <paulmck@linux.ibm.com>
>> > Cc: "Joel Fernandes (Google)" <joel@joelfernandes.org>
>> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> > Cc: Thomas Gleixner <tglx@linutronix.de>
>> > Cc: oprofile-list@lists.sf.net
>> > Cc: linux-kernel@vger.kernel.org
>> > Cc: netdev@vger.kernel.org
>> > Cc: bpf@vger.kernel.org
>> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
>> 
>> 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/3] module: Fix up module_notifier return values.
  2019-06-24 14:01   ` Mathieu Desnoyers
  2019-06-24 15:52     ` Joel Fernandes
@ 2019-06-24 20:58     ` Frank Ch. Eigler
  2019-06-25  7:42       ` Peter Zijlstra
  1 sibling, 1 reply; 10+ messages in thread
From: Frank Ch. Eigler @ 2019-06-24 20:58 UTC (permalink / raw)
  To: Mathieu Desnoyers
  Cc: Peter Zijlstra, Jessica Yu, linux-kernel, Josh Poimboeuf, jikos,
	mbenes, Petr Mladek, Alexei Starovoitov, Daniel Borkmann,
	Andrew Morton, Robert Richter, rostedt, Ingo Molnar,
	Martin KaFai Lau, Song Liu, Yonghong Song, paulmck,
	Joel Fernandes, Google, Ard Biesheuvel, Thomas Gleixner,
	oprofile-list, netdev, bpf

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/3] module: Fix up module_notifier return values.
  2019-06-24 20:58     ` Frank Ch. Eigler
@ 2019-06-25  7:42       ` Peter Zijlstra
  2019-07-04 12:48         ` Robert Richter
  0 siblings, 1 reply; 10+ messages in thread
From: Peter Zijlstra @ 2019-06-25  7:42 UTC (permalink / raw)
  To: Frank Ch. Eigler
  Cc: Mathieu Desnoyers, Jessica Yu, linux-kernel, Josh Poimboeuf,
	jikos, mbenes, Petr Mladek, Alexei Starovoitov, Daniel Borkmann,
	Andrew Morton, Robert Richter, rostedt, Ingo Molnar,
	Martin KaFai Lau, Song Liu, Yonghong Song, paulmck,
	Joel Fernandes, Google, Ard Biesheuvel, Thomas Gleixner,
	oprofile-list, netdev, bpf

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.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/3] module: Fix up module_notifier return values.
  2019-06-24  9:18 ` [PATCH 2/3] module: Fix up module_notifier return values Peter Zijlstra
  2019-06-24 14:01   ` Mathieu Desnoyers
@ 2019-07-04 12:34   ` Robert Richter
  1 sibling, 0 replies; 10+ messages in thread
From: Robert Richter @ 2019-07-04 12:34 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Jessica Yu, linux-kernel, jpoimboe, jikos, mbenes, pmladek, ast,
	daniel, akpm, Steven Rostedt, Ingo Molnar, Martin KaFai Lau,
	Song Liu, Yonghong Song, Mathieu Desnoyers, Paul E. McKenney,
	Joel Fernandes (Google),
	Ard Biesheuvel, Thomas Gleixner, oprofile-list, netdev, bpf

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 <rric@kernel.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Martin KaFai Lau <kafai@fb.com>
> Cc: Song Liu <songliubraving@fb.com>
> Cc: Yonghong Song <yhs@fb.com>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: "Paul E. McKenney" <paulmck@linux.ibm.com>
> Cc: "Joel Fernandes (Google)" <joel@joelfernandes.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: oprofile-list@lists.sf.net
> Cc: linux-kernel@vger.kernel.org
> Cc: netdev@vger.kernel.org
> Cc: bpf@vger.kernel.org
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  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 <rric@kernel.org>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/3] module: Fix up module_notifier return values.
  2019-06-25  7:42       ` Peter Zijlstra
@ 2019-07-04 12:48         ` Robert Richter
  0 siblings, 0 replies; 10+ messages in thread
From: Robert Richter @ 2019-07-04 12:48 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Frank Ch. Eigler, Mathieu Desnoyers, Jessica Yu, linux-kernel,
	Josh Poimboeuf, jikos, mbenes, Petr Mladek, Alexei Starovoitov,
	Daniel Borkmann, Andrew Morton, rostedt, Ingo Molnar,
	Martin KaFai Lau, Song Liu, Yonghong Song, paulmck,
	Joel Fernandes, Google, Ard Biesheuvel, Thomas Gleixner,
	oprofile-list, netdev, bpf

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2019-07-04 12:48 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20190624091843.859714294@infradead.org>
2019-06-24  9:18 ` [PATCH 2/3] module: Fix up module_notifier return values Peter Zijlstra
2019-06-24 14:01   ` Mathieu Desnoyers
2019-06-24 15:52     ` Joel Fernandes
2019-06-24 17:50       ` Mathieu Desnoyers
2019-06-24 20:58     ` Frank Ch. Eigler
2019-06-25  7:42       ` Peter Zijlstra
2019-07-04 12:48         ` Robert Richter
2019-07-04 12:34   ` Robert Richter
2019-06-24  9:18 ` [PATCH 3/3] module: Properly propagate MODULE_STATE_COMING failure Peter Zijlstra
2019-06-24 11:07   ` Peter Zijlstra

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).