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

[Josh Poimboeuf]

On Fri, Jan 29, 2016 at 08:25:15PM +0100, Miroslav Benes wrote:
> On Fri, 29 Jan 2016, Josh Poimboeuf wrote:
> 
> > On Fri, Jan 29, 2016 at 12:40:14PM -0500, Steven Rostedt wrote:
> > > [ Added Rusty, as he's still maintainer of the module code ]
> > > 
> > > On Fri, 29 Jan 2016 11:30:10 -0600
> > > Josh Poimboeuf <jpoimboe@xxxxxxxxxx> 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.
> > 
> > Agreed, and we might as well make this change now to avoid more churn
> > later.
> 
> It is possible to achieve the same goal even with the notifiers. They are 
> processed synchronously in complete_formation(). So we can put our klp 
> hook after that, right? Or better, put it to load_module() after 
> complete_formation() call. There is an error handling code even today 
> (that is, parse_args() or mod_sysfs_setup() can fail). Moreover, we'll 
> have a hook there with Jessica's relocation rework patch set.

Well, my feeling is that we should really apply livepatch relocations
before allowing any other notifiers to run, in case the relocations
affect them.  But it's just a feeling; I don't have any specific
examples to justify it (yet).

> But Steven's reasoning is convincing, so I'm all up for it.
> 
> Regards,
> Miroslav

-- 
Josh