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

[Seth Jennings <sjenning@redhat.com>]

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.

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.

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

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

Thanks,
Seth

> 
> Best Regards,
> Petr