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

[Miroslav Benes <mbenes@suse.cz>]

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.

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.

Miroslav