Re: [PATCH 2/3] module: Fix up module_notifier return values.

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

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