On Fri, Nov 07, 2014 at 07:40:11PM +0100, Petr Mladek wrote:
> 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.
I agree that would be better.
>
>
> > 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.
Yes (Josh just brought this to my attention) is that the notifiers are
call with GOING _after_ the module's exit function is called.
Thanks,
Seth
>
>
> > >
> > > > + 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
--
To unsubscribe from this list: send the line "unsubscribe live-patching" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
