Re: [RFC][PATCH] module: Propagate MODULE_STATE_COMING notifier errors
From: Miroslav Benes
Date: Wed Jun 19 2019 - 07:17:22 EST
On Mon, 17 Jun 2019, Peter Zijlstra wrote:
> Some module notifiers; such as jump_label_module_notifier(),
> tracepoint_module_notify(); can fail the MODULE_STATE_COMING callback
> (due to -ENOMEM for example). However module.c:prepare_coming_module()
> ignores all such errors, even though this function can already fail due
> to klp_module_coming().
It does, but there is no change from the pre-prepare_coming_module()
times. Coming notifiers were called in complete_formation(), their return
values happily ignored and going notifiers not called to clean up even
> Therefore, propagate the notifier error and ensure we call the GOING
> notifier when we do fail, to ensure cleanup for all notifiers that
> didn't fail. Auditing all notifiers to make sure calling GOING without
> COMING first is OK found no obvious problems with that, but it did find
> a whole bunch of issues with return values, so clean those up too.
Jessica, do you know why coming notifiers do not return errors without
this patch (or to be precise, blocking_notifier_call_chain() return value
is not taken into the account)? We have come across the issue couple of
times already and I think there was a reason, but I cannot remember
anything and the code does not help either.
Also the situation around the return values themselves is not completely
clear. If there is no NOTIFY_STOP_MASK set, only the return value of the
last notifier called is returned, so good that you checked, Peter.
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3638,9 +3638,10 @@ static int prepare_coming_module(struct module *mod)
> if (err)
> return err;
> - blocking_notifier_call_chain(&module_notify_list,
> - MODULE_STATE_COMING, mod);
> - return 0;
> + ret = blocking_notifier_call_chain(&module_notify_list,
> + MODULE_STATE_COMING, mod);
> + ret = notifier_to_errno(ret);
> + return ret;
> static int unknown_module_param_cb(char *param, char *val, const char *modname,
> @@ -3780,7 +3781,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
> err = prepare_coming_module(mod);
> if (err)
> - goto bug_cleanup;
> + goto coming_cleanup;
Not good. klp_module_going() is not prepared to be called without
klp_module_coming() succeeding. "Funny" things might happen.
Also destroy_params() might be called without parse_args() first now.
So it calls for more fine-grained error handling.