Re: module notifier: was Re: [PATCH 2/2] kernel: add support for live patching

[Petr Mladek <pmladek@suse.cz>]

On Fri 2014-11-07 12:07:11, Seth Jennings wrote:
> On Fri, Nov 07, 2014 at 06:13:07PM +0100, Petr Mladek wrote:
> > On Thu 2014-11-06 08:39:08, Seth Jennings wrote:
> > > This commit introduces code for the live patching core.  It implements
> > > an ftrace-based mechanism and kernel interface for doing live patching
> > > of kernel and kernel module functions.
> > > 
> > > It represents the greatest common functionality set between kpatch and
> > > kgraft and can accept patches built using either method.
> > > 
> > > This first version does not implement any consistency mechanism that
> > > ensures that old and new code do not run together.  In practice, ~90% of
> > > CVEs are safe to apply in this way, since they simply add a conditional
> > > check.  However, any function change that can not execute safely with
> > > the old version of the function can _not_ be safely applied in this
> > > version.
> > 
> > [...]
> >  
> > > +/******************************
> > > + * module notifier
> > > + *****************************/
> > > +
> > > +static int lp_module_notify(struct notifier_block *nb, unsigned long action,
> > > +			    void *data)
> > > +{
> > > +	struct module *mod = data;
> > > +	struct lpc_patch *patch;
> > > +	struct lpc_object *obj;
> > > +	int ret = 0;
> > > +
> > > +	if (action != MODULE_STATE_COMING)
> > > +		return 0;
> > 
> > IMHO, we should handle also MODULE_STATE_GOING. We should unregister
> > the ftrace handlers and update the state of the affected objects
> > (ENABLED -> DISABLED)
> 
> The mechanism we use to avoid this right now is taking a reference on
> patched module.  We only release that reference after the patch is
> disabled, which unregisters all the patched functions from ftrace.

I see. This was actually another thing that I noticed and wanted to
investigate :-) I think that we should not force users to disable
the entire patch if they want to remove some module.


> However, your comment reminded me of an idea I had to use
> MODULE_STATE_GOING and let the lpc_mutex protect against races.  I think
> it could be cleaner, but I haven't fleshed the idea out fully.

AFAIK, the going module is not longer used when the notifier is
called. Therefore we could remove the patch fast way even when
patching would require the slow path otherwise.


> > 
> > > +	down(&lpc_mutex);
> > > +
> > > +	list_for_each_entry(patch, &lpc_patches, list) {
> > > +		if (patch->state == DISABLED)
> > > +			continue;
> > > +		list_for_each_entry(obj, &patch->objs, list) {
> > > +			if (strcmp(obj->name, mod->name))
> > > +				continue;
> > > +			pr_notice("load of module '%s' detected, applying patch '%s'\n",
> > > +				  mod->name, patch->mod->name);
> > > +			obj->mod = mod;
> > > +			ret = lpc_enable_object(patch->mod, obj);
> > > +			if (ret)
> > > +				goto out;
> > > +			break;
> > > +		}
> > > +	}
> > > +
> > > +	up(&lpc_mutex);
> > > +	return 0;
> > > +out:
> > 
> > I would name this err_our or so to make it clear that it is used when
> > something fails.
> 
> Just "err" good?

Fine with me.
 
> > > +	up(&lpc_mutex);
> > > +	WARN("failed to apply patch '%s' to module '%s'\n",
> > > +		patch->mod->name, mod->name);
> > > +	return 0;
> > > +}
> > > +
> > > +static struct notifier_block lp_module_nb = {
> > > +	.notifier_call = lp_module_notify,
> > > +	.priority = INT_MIN, /* called last */
> > 
> > The handler for MODULE_STATE_COMMING would need have higger priority,
> > if we want to cleanly unregister the ftrace handlers.
> 
> Yes, we might need two handlers at different priorities if we decide to
> go that direction: one for MODULE_STATE_GOING at high/max and one for
> MODULE_STATE_COMING at low/min.

kGraft has notifier only for the going state. The initialization is
called directly from load_module() after ftrace_module_init()
and complete_formation() before it is executed by parse_args().

I need to investigate if the notifier is more elegant and safe or not.

Best Regards,
Petr