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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
