From: Minfei Huang <[email protected]>
Livepatch will obey the stacking rule to enable/disable the patch. It
only allows to enable the patch, when it is the fist disabled patch,
disable the patch, when it is the last enabled patch.
In the livepatch code, it uses list to gather the all of the patches.
And we do not know whether the previous/next patch is patched to the
same modules or vmlinux in that way.
According to above rule, livepatch will make incorrect decision to
enable/disable the patch. Following is an example to show how livepatch
does.
- install the livepatch example module which is in samples/livepatch.
- install the third part kernel module
- install the livepatch module which is patched to the third part module
- disable the livepatch example module
We can find that we can not disable livepatch example module, although
it is the last enabled patch.
To fix this issue, we will find the corresponding patch which is patched
to the same modules or vmlinux, when we enable/disable the patch.
Signed-off-by: Minfei Huang <[email protected]>
---
kernel/livepatch/core.c | 55 +++++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 51 insertions(+), 4 deletions(-)
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 6e53441..d59aec7 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -429,6 +429,27 @@ err:
return ret;
}
+static bool is_patched_to_same(struct klp_patch *p1, struct klp_patch *p2)
+{
+ struct klp_object *obj1, *obj2;
+ bool is_mod1, is_mod2;
+
+ klp_for_each_object(p1, obj1) {
+ klp_for_each_object(p2, obj2) {
+ is_mod1 = !!klp_is_module(obj1);
+ is_mod2 = !!klp_is_module(obj2);
+
+ if (is_mod1 && is_mod2) {
+ if (!strcmp(obj1->name, obj2->name))
+ return true;
+ } else if (!is_mod1 && !is_mod2)
+ return true;
+ }
+ }
+
+ return false;
+}
+
static void klp_disable_object(struct klp_object *obj)
{
struct klp_func *func;
@@ -463,13 +484,26 @@ static int klp_enable_object(struct klp_object *obj)
return 0;
}
+struct klp_patch *get_next_patch(struct klp_patch *patch)
+{
+ struct klp_patch *p = patch;
+
+ list_for_each_entry_continue(p, &klp_patches, list) {
+ if (is_patched_to_same(p, patch))
+ return p;
+ }
+
+ return NULL;
+}
+
static int __klp_disable_patch(struct klp_patch *patch)
{
+ struct klp_patch *p;
struct klp_object *obj;
/* enforce stacking: only the last enabled patch can be disabled */
- if (!list_is_last(&patch->list, &klp_patches) &&
- list_next_entry(patch, list)->state == KLP_ENABLED)
+ p = get_next_patch(patch);
+ if (p && (p->state == KLP_ENABLED))
return -EBUSY;
pr_notice("disabling patch '%s'\n", patch->mod->name);
@@ -516,8 +550,21 @@ err:
}
EXPORT_SYMBOL_GPL(klp_disable_patch);
+struct klp_patch *get_prev_patch(struct klp_patch *patch)
+{
+ struct klp_patch *p = patch;
+
+ list_for_each_entry_continue_reverse(p, &klp_patches, list) {
+ if (is_patched_to_same(p, patch))
+ return p;
+ }
+
+ return NULL;
+}
+
static int __klp_enable_patch(struct klp_patch *patch)
{
+ struct klp_patch *p;
struct klp_object *obj;
int ret;
@@ -525,8 +572,8 @@ static int __klp_enable_patch(struct klp_patch *patch)
return -EINVAL;
/* enforce stacking: only the first disabled patch can be enabled */
- if (patch->list.prev != &klp_patches &&
- list_prev_entry(patch, list)->state == KLP_DISABLED)
+ p = get_prev_patch(patch);
+ if (p && (p->state == KLP_DISABLED))
return -EBUSY;
pr_notice_once("tainting kernel with TAINT_LIVEPATCH\n");
--
2.2.2
Could someone help to review this patch?
Thanks
Minfei
On 07/15/15 at 04:55pm, Minfei Huang wrote:
> From: Minfei Huang <[email protected]>
>
> Livepatch will obey the stacking rule to enable/disable the patch. It
> only allows to enable the patch, when it is the fist disabled patch,
> disable the patch, when it is the last enabled patch.
>
> In the livepatch code, it uses list to gather the all of the patches.
> And we do not know whether the previous/next patch is patched to the
> same modules or vmlinux in that way.
>
> According to above rule, livepatch will make incorrect decision to
> enable/disable the patch. Following is an example to show how livepatch
> does.
>
> - install the livepatch example module which is in samples/livepatch.
> - install the third part kernel module
> - install the livepatch module which is patched to the third part module
> - disable the livepatch example module
>
> We can find that we can not disable livepatch example module, although
> it is the last enabled patch.
>
> To fix this issue, we will find the corresponding patch which is patched
> to the same modules or vmlinux, when we enable/disable the patch.
>
> Signed-off-by: Minfei Huang <[email protected]>
> ---
> kernel/livepatch/core.c | 55 +++++++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 51 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 6e53441..d59aec7 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -429,6 +429,27 @@ err:
> return ret;
> }
>
> +static bool is_patched_to_same(struct klp_patch *p1, struct klp_patch *p2)
> +{
> + struct klp_object *obj1, *obj2;
> + bool is_mod1, is_mod2;
> +
> + klp_for_each_object(p1, obj1) {
> + klp_for_each_object(p2, obj2) {
> + is_mod1 = !!klp_is_module(obj1);
> + is_mod2 = !!klp_is_module(obj2);
> +
> + if (is_mod1 && is_mod2) {
> + if (!strcmp(obj1->name, obj2->name))
> + return true;
> + } else if (!is_mod1 && !is_mod2)
> + return true;
> + }
> + }
> +
> + return false;
> +}
> +
> static void klp_disable_object(struct klp_object *obj)
> {
> struct klp_func *func;
> @@ -463,13 +484,26 @@ static int klp_enable_object(struct klp_object *obj)
> return 0;
> }
>
> +struct klp_patch *get_next_patch(struct klp_patch *patch)
> +{
> + struct klp_patch *p = patch;
> +
> + list_for_each_entry_continue(p, &klp_patches, list) {
> + if (is_patched_to_same(p, patch))
> + return p;
> + }
> +
> + return NULL;
> +}
> +
> static int __klp_disable_patch(struct klp_patch *patch)
> {
> + struct klp_patch *p;
> struct klp_object *obj;
>
> /* enforce stacking: only the last enabled patch can be disabled */
> - if (!list_is_last(&patch->list, &klp_patches) &&
> - list_next_entry(patch, list)->state == KLP_ENABLED)
> + p = get_next_patch(patch);
> + if (p && (p->state == KLP_ENABLED))
> return -EBUSY;
>
> pr_notice("disabling patch '%s'\n", patch->mod->name);
> @@ -516,8 +550,21 @@ err:
> }
> EXPORT_SYMBOL_GPL(klp_disable_patch);
>
> +struct klp_patch *get_prev_patch(struct klp_patch *patch)
> +{
> + struct klp_patch *p = patch;
> +
> + list_for_each_entry_continue_reverse(p, &klp_patches, list) {
> + if (is_patched_to_same(p, patch))
> + return p;
> + }
> +
> + return NULL;
> +}
> +
> static int __klp_enable_patch(struct klp_patch *patch)
> {
> + struct klp_patch *p;
> struct klp_object *obj;
> int ret;
>
> @@ -525,8 +572,8 @@ static int __klp_enable_patch(struct klp_patch *patch)
> return -EINVAL;
>
> /* enforce stacking: only the first disabled patch can be enabled */
> - if (patch->list.prev != &klp_patches &&
> - list_prev_entry(patch, list)->state == KLP_DISABLED)
> + p = get_prev_patch(patch);
> + if (p && (p->state == KLP_DISABLED))
> return -EBUSY;
>
> pr_notice_once("tainting kernel with TAINT_LIVEPATCH\n");
> --
> 2.2.2
>
On Wed, Jul 15, 2015 at 04:55:06PM +0800, Minfei Huang wrote:
> From: Minfei Huang <[email protected]>
>
> Livepatch will obey the stacking rule to enable/disable the patch. It
> only allows to enable the patch, when it is the fist disabled patch,
> disable the patch, when it is the last enabled patch.
>
> In the livepatch code, it uses list to gather the all of the patches.
> And we do not know whether the previous/next patch is patched to the
> same modules or vmlinux in that way.
>
> According to above rule, livepatch will make incorrect decision to
> enable/disable the patch. Following is an example to show how livepatch
> does.
>
> - install the livepatch example module which is in samples/livepatch.
> - install the third part kernel module
> - install the livepatch module which is patched to the third part module
> - disable the livepatch example module
>
> We can find that we can not disable livepatch example module, although
> it is the last enabled patch.
>
> To fix this issue, we will find the corresponding patch which is patched
> to the same modules or vmlinux, when we enable/disable the patch.
Is it really safe to assume that there are no dependencies between
patches which patch different objects?
--
Josh
On 07/22/15 at 09:40am, Josh Poimboeuf wrote:
> On Wed, Jul 15, 2015 at 04:55:06PM +0800, Minfei Huang wrote:
> > From: Minfei Huang <[email protected]>
> >
> > Livepatch will obey the stacking rule to enable/disable the patch. It
> > only allows to enable the patch, when it is the fist disabled patch,
> > disable the patch, when it is the last enabled patch.
> >
> > In the livepatch code, it uses list to gather the all of the patches.
> > And we do not know whether the previous/next patch is patched to the
> > same modules or vmlinux in that way.
> >
> > According to above rule, livepatch will make incorrect decision to
> > enable/disable the patch. Following is an example to show how livepatch
> > does.
> >
> > - install the livepatch example module which is in samples/livepatch.
> > - install the third part kernel module
> > - install the livepatch module which is patched to the third part module
> > - disable the livepatch example module
> >
> > We can find that we can not disable livepatch example module, although
> > it is the last enabled patch.
> >
> > To fix this issue, we will find the corresponding patch which is patched
> > to the same modules or vmlinux, when we enable/disable the patch.
>
> Is it really safe to assume that there are no dependencies between
> patches which patch different objects?
>
I think so.
It is weird that there are denpendencies which are different objects in
different patches. To apply this patch, we can make livepatch more
flexible to manage the patch.
Thanks
Minfei
> --
> Josh
On Thu, Jul 23, 2015 at 12:02:06PM +0800, Minfei Huang wrote:
> On 07/22/15 at 09:40am, Josh Poimboeuf wrote:
> > Is it really safe to assume that there are no dependencies between
> > patches which patch different objects?
> >
>
> I think so.
What about the following scenario:
1. register and enable patch A, which patches vmlinux_func() and changes
its call signature
2. register and enable patch B, which patches a (not yet loaded) module
M so that it will call vmlinux_func() with its new call signature
3. load module M, which is immediately patched by patch B
4. disable patch A. Now the patched module M calls the unpatched
version of vmlinux_func() with the wrong call signature - BOOM
In this case B, a patch to a module, would have an implicit dependency
on A, a patch to vmlinux.
So I don't think the approach in the above patch would work. But I *do*
think we may need to revisit how we handle dependencies...
Note that our current patch stacking protects against unloading out of
order, but it assumes that the user loaded them in the correct order in
the first place. If M and B are loaded before A, then it would still go
boom even with today's code.
So IMO the way we handle dependencies today is incomplete. Some options
for improvement are:
a) Don't allow dependencies between patches. Instead all dependencies
must be contained within the patch itself. So patch A and patch B
are combined into a single patch AB. If, later, a new patch C is
needed, which also depends on A, then create a new cumulative patch
ABC which replaces AB.
Note there's no way to enforce the fact there are no dependencies,
because they can be hidden. So it would just have to be a documented
rule that the patch author must follow, as part of the (yet to be
written) patch creation guidelines. This actually isn't a big deal
because there are several other (still undocumented) rules the patch
author must already follow.
This would mean that klp code can assume there are no dependencies,
and so patch stacking would no longer be necessary. We'd probably
have to rework the ops->func_stack code a bit so that it's ordered by
when the patches were registered instead of when they were enabled,
so that disabling and re-enabling an older patch wouldn't override a
newer cumulative one which replaces it.
b) Create a way for the patch author to specify explicit patch
dependencies.
Note that both options a and b delegate responsibility to the patch
author to ensure that dependencies are handled appropriately.
Ultimately I don't think there's any way around that.
My vote would be option a for now, by removing patch stacking and
documenting the guidelines. With the eventual possibility of adding b
if needed.
--
Josh
On 07/23/15 at 01:07pm, Josh Poimboeuf wrote:
> On Thu, Jul 23, 2015 at 12:02:06PM +0800, Minfei Huang wrote:
> > On 07/22/15 at 09:40am, Josh Poimboeuf wrote:
> > > Is it really safe to assume that there are no dependencies between
> > > patches which patch different objects?
> > >
> >
> > I think so.
>
> What about the following scenario:
>
> 1. register and enable patch A, which patches vmlinux_func() and changes
> its call signature
> 2. register and enable patch B, which patches a (not yet loaded) module
> M so that it will call vmlinux_func() with its new call signature
> 3. load module M, which is immediately patched by patch B
> 4. disable patch A. Now the patched module M calls the unpatched
> version of vmlinux_func() with the wrong call signature - BOOM
>
> In this case B, a patch to a module, would have an implicit dependency
> on A, a patch to vmlinux.
>
> So I don't think the approach in the above patch would work. But I *do*
> think we may need to revisit how we handle dependencies...
>
> Note that our current patch stacking protects against unloading out of
> order, but it assumes that the user loaded them in the correct order in
> the first place. If M and B are loaded before A, then it would still go
> boom even with today's code.
>
> So IMO the way we handle dependencies today is incomplete. Some options
> for improvement are:
>
> a) Don't allow dependencies between patches. Instead all dependencies
> must be contained within the patch itself. So patch A and patch B
> are combined into a single patch AB. If, later, a new patch C is
> needed, which also depends on A, then create a new cumulative patch
> ABC which replaces AB.
>
> Note there's no way to enforce the fact there are no dependencies,
> because they can be hidden. So it would just have to be a documented
> rule that the patch author must follow, as part of the (yet to be
> written) patch creation guidelines. This actually isn't a big deal
> because there are several other (still undocumented) rules the patch
> author must already follow.
>
> This would mean that klp code can assume there are no dependencies,
> and so patch stacking would no longer be necessary. We'd probably
> have to rework the ops->func_stack code a bit so that it's ordered by
> when the patches were registered instead of when they were enabled,
> so that disabling and re-enabling an older patch wouldn't override a
> newer cumulative one which replaces it.
>
> b) Create a way for the patch author to specify explicit patch
> dependencies.
>
> Note that both options a and b delegate responsibility to the patch
> author to ensure that dependencies are handled appropriately.
> Ultimately I don't think there's any way around that.
>
> My vote would be option a for now, by removing patch stacking and
> documenting the guidelines. With the eventual possibility of adding b
> if needed.
Thanks for your explaination.
Yes, kernel may crash, if module M calls the unpatched and exported
function vmlinux_func.
I may prefer to choice B, since user can make their own rule to restrict
the patches enabled/disabled. Thus livepatch may be simplier in code
layer.
Thanks
Minfei
On Thu, 23 Jul 2015, Josh Poimboeuf wrote:
> a) Don't allow dependencies between patches. Instead all dependencies
> must be contained within the patch itself. So patch A and patch B
> are combined into a single patch AB. If, later, a new patch C is
> needed, which also depends on A, then create a new cumulative patch
> ABC which replaces AB.
>
> Note there's no way to enforce the fact there are no dependencies,
> because they can be hidden. So it would just have to be a documented
> rule that the patch author must follow, as part of the (yet to be
> written) patch creation guidelines. This actually isn't a big deal
> because there are several other (still undocumented) rules the patch
> author must already follow.
>
> This would mean that klp code can assume there are no dependencies,
> and so patch stacking would no longer be necessary. We'd probably
> have to rework the ops->func_stack code a bit so that it's ordered by
> when the patches were registered instead of when they were enabled,
> so that disabling and re-enabling an older patch wouldn't override a
> newer cumulative one which replaces it.
>
> b) Create a way for the patch author to specify explicit patch
> dependencies.
>
> Note that both options a and b delegate responsibility to the patch
> author to ensure that dependencies are handled appropriately. Ultimately
> I don't think there's any way around that.
>
> My vote would be option a for now, by removing patch stacking and
> documenting the guidelines. With the eventual possibility of adding b
> if needed.
As a data point, option (A) more or less describes the way how we in SUSE
are distributing the actual live patches -- i.e. there is always a single
cummulative patch package, that contains all the patches "squashed"
together.
It's a nice property of kGraft-like patching that the time required for
the patching to finish doesn't depend on the number of functions being
patched (because it's O(#processess_in_kernel)).
That being said, I am also for option (A), but we have to keep in mind
that time needed by some consistency models (those which are
O(#patched_functions)) to finalize might be negatively affected by it.
Thanks,
--
Jiri Kosina
SUSE Labs