Re: module notifier: was Re: [PATCH 2/2] kernel: add support for live patching
On Tue, Nov 11, 2014 at 11:17:39PM +0100, Jiri Kosina wrote: > On Tue, 11 Nov 2014, Seth Jennings wrote: > > > It will be in v2 (hopefully out in the next couple of days). > > FWIW we are also working on a few patches on top of v1 to back some of the > proposals we've made during the first round of review, so maybe it might > make sense to wait with v2 a little bit more, so that it incorporates as > much v1 feedback as possible ... ? What proposals in particular? I've already made many of the changes that we agreed upon. Thanks, Seth > > Thanks, > > -- > Jiri Kosina > SUSE Labs -- 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/
Re: module notifier: was Re: [PATCH 2/2] kernel: add support for live patching
On Tue, 11 Nov 2014, Seth Jennings wrote: > It will be in v2 (hopefully out in the next couple of days). FWIW we are also working on a few patches on top of v1 to back some of the proposals we've made during the first round of review, so maybe it might make sense to wait with v2 a little bit more, so that it incorporates as much v1 feedback as possible ... ? Thanks, -- Jiri Kosina SUSE Labs -- 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/
Re: module notifier: was Re: [PATCH 2/2] kernel: add support for live patching
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:
[...]
> > > > + 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.
I looked it up and having a COMING notifier with priority INT_MIN is
effectively the same as having a call between complete_formation() and
parse_args() since the notifiers are called as the last thing in
complete_formation().
I think I've found a clean way to avoid the ref taking on the patched
modules using only the notifier and lpc_mutex. It will be in v2
(hopefully out in the next couple of days).
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/
Re: module notifier: was Re: [PATCH 2/2] kernel: add support for live patching
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 linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info a
Re: module notifier: was Re: [PATCH 2/2] kernel: add support for live patching
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
--
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/
Re: module notifier: was Re: [PATCH 2/2] kernel: add support for live patching
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/
