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