Re: [PATCH 2/3] module: Fix up module_notifier return values.
From: Joel Fernandes
Date: Mon Jun 24 2019 - 11:52:18 EST
On Mon, Jun 24, 2019 at 10:01:04AM -0400, Mathieu Desnoyers wrote:
> ----- On Jun 24, 2019, at 5:18 AM, Peter Zijlstra peterz@xxxxxxxxxxxxx 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@xxxxxxxxxx>
> > Cc: Steven Rostedt <rostedt@xxxxxxxxxxx>
> > Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> > Cc: Alexei Starovoitov <ast@xxxxxxxxxx>
> > Cc: Daniel Borkmann <daniel@xxxxxxxxxxxxx>
> > Cc: Martin KaFai Lau <kafai@xxxxxx>
> > Cc: Song Liu <songliubraving@xxxxxx>
> > Cc: Yonghong Song <yhs@xxxxxx>
> > Cc: Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx>
> > Cc: "Paul E. McKenney" <paulmck@xxxxxxxxxxxxx>
> > Cc: "Joel Fernandes (Google)" <joel@xxxxxxxxxxxxxxxxx>
> > Cc: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
> > Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> > Cc: oprofile-list@xxxxxxxxxxxx
> > Cc: linux-kernel@xxxxxxxxxxxxxxx
> > Cc: netdev@xxxxxxxxxxxxxxx
> > Cc: bpf@xxxxxxxxxxxxxxx
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> 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
> 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@xxxxxxxxxxxx>
Looks good to me too.
Reviewed-by: Joel Fernandes (Google) <joel@xxxxxxxxxxxxxxxxx>
Could we CC stable so that the fix is propagated to older kernels?
> 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.
> commit 5eac9d146a7d947f0f314c4f7103c92cbccaeaf3
> Author: Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx>
> 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
> 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
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx>
> 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)
> - return 0;
> + return NOTIFY_OK;
> Mathieu Desnoyers
> EfficiOS Inc.