+++ Miroslav Benes [19/06/19 13:12 +0200]:
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
before.
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.
I tried to do some digging but did not find a specific reason why the
return value is not taken into account. I don't think it was ever
considered. I traced it back to a commit in 2003 that introduced the
coming notifier (84486c2e135 "module load notification" in the history
repo), but even there the return value is ignored. After grepping
around it seems most usages of blocking_notifier_call_chain() just
ignore the return value.
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.
I would not mind if prepare_coming_module() was taken apart to handle the more
fine-grained error handling. Maybe something like (untested and unreviewed):
diff --git a/kernel/module.c b/kernel/module.c
index c1517053e9d6..9e470f9ae0a5 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3799,10 +3799,16 @@ static int load_module(struct load_info *info, const
char __user *uargs,
if (err)
goto ddebug_cleanup;
- err = prepare_coming_module(mod);
+ ftrace_module_enable(mod);
+ err = klp_module_coming(mod);
if (err)
goto bug_cleanup;
+ err = blocking_notifier_call_chain(&module_notify_list,
+ MODULE_STATE_COMING, mod);
+ if (err)
+ goto notifier_cleanup;
+
/* Module is ready to execute: parsing args may do that. */
after_dashes = parse_args(mod->name, mod->args, mod->kp, mod->num_kp,
-32768, 32767, mod,
@@ -3837,8 +3843,9 @@ static int load_module(struct load_info *info, const char
__user *uargs,
sysfs_cleanup:
mod_sysfs_teardown(mod);
coming_cleanup:
- mod->state = MODULE_STATE_GOING;
destroy_params(mod->kp, mod->num_kp);
+notifier_cleanup:
+ mod->state = MODULE_STATE_GOING;
blocking_notifier_call_chain(&module_notify_list,
MODULE_STATE_GOING, mod);
klp_module_going(mod);
But I think we could also still keep everything in prepare_coming_module() if
the klp hooks do get converted to notifiers.
Thanks,
Jessica
