Re: [RFC][PATCH] module: Propagate MODULE_STATE_COMING notifier errors

[Jessica Yu <jeyu@kernel.org>]

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