linux-kernel - Re: [PATCH 1/2] livepatch: Implement separate coming and going module notifiers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160129124014.204020ed@gandalf.local.home>
Date:	Fri, 29 Jan 2016 12:40:14 -0500
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Josh Poimboeuf <jpoimboe@...hat.com>
Cc:	Miroslav Benes <mbenes@...e.cz>, Jessica Yu <jeyu@...hat.com>,
	Seth Jennings <sjenning@...hat.com>,
	Jiri Kosina <jikos@...nel.org>,
	Vojtech Pavlik <vojtech@...e.com>,
	Ingo Molnar <mingo@...hat.com>, live-patching@...r.kernel.org,
	linux-kernel@...r.kernel.org, Rusty Russell <rusty@...tcorp.com.au>
Subject: Re: [PATCH 1/2] livepatch: Implement separate coming and going
 module notifiers

[ Added Rusty, as he's still maintainer of the module code ]

On Fri, 29 Jan 2016 11:30:10 -0600
Josh Poimboeuf <jpoimboe@...hat.com> wrote:

> On Fri, Jan 29, 2016 at 05:30:46PM +0100, Miroslav Benes wrote:
> > Otherwise than that it looks good. I agree there are advantages to split 
> > the notifiers. For example we can replace the coming one with the function 
> > call somewhere in load_module() to improve error handling if the patching 
> > fails while loading a module. This would be handy with a consistency model 
> > in the future.  
> 
> Yeah, we'll need something like that eventually.  Though we'll need to
> make sure that ftrace_module_enable() is still called beforehand, after
> setting MODULE_STATE_COMING state, due to the race described in 5156dca.
> 
> Something like:
> 
> [note: klp_module_notify_coming() is replaced with klp_module_enable()]
> 
> diff --git a/kernel/module.c b/kernel/module.c
> index 8358f46..aeabd81 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3371,6 +3371,13 @@ static int complete_formation(struct module *mod, struct load_info *info)
>  	mod->state = MODULE_STATE_COMING;
>  	mutex_unlock(&module_mutex);
>  
> +	ftrace_module_enable(mod);
> +	err = klp_module_enable(mod);
> +	if (err) {
> +		ftrace_release_mod(mod);
> +		return err;
> +	}
> +
>  	blocking_notifier_call_chain(&module_notify_list,
>  				     MODULE_STATE_COMING, mod);
>  	return 0;
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index eca592f..c42cf37 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -5045,9 +5045,6 @@ static int ftrace_module_notify(struct notifier_block *self,
>  	struct module *mod = data;
>  
>  	switch (val) {
> -	case MODULE_STATE_COMING:
> -		ftrace_module_enable(mod);
> -		break;
>  	case MODULE_STATE_GOING:
>  		ftrace_release_mod(mod);
>  		break;

If we end up doing something like this, I would just say punt and have
the ftrace code be hardcoded into the module code and remove the
notifiers completely. ftrace (and live kernel patching for that matter)
are rather special. They are not a filesystem or driver. They are core
utilities and having them called directly from the module code may be
prudent and better to understand and control.

Note, you still need to have prototypes for ftrace_module_enable() and
a stub when ftrace is not configured. Same goes for klp_module_enable().

-- Steve