Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1692196 > unrolled thread

Re: [PATCH] livepatch: add (un)patch hooks

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] livepatch: add (un)patch hooks Josh Poimboeuf <jpoimboe@redhat.com> - 2017-07-19 23:00 +0200
    Re: [PATCH] livepatch: add (un)patch hooks Josh Poimboeuf <jpoimboe@redhat.com> - 2017-07-20 06:20 +0200
      Re: [PATCH] livepatch: add (un)patch hooks Josh Poimboeuf <jpoimboe@redhat.com> - 2017-07-20 06:30 +0200
      Re: [PATCH] livepatch: add (un)patch hooks Josh Poimboeuf <jpoimboe@redhat.com> - 2017-07-20 06:40 +0200
      Re: [PATCH] livepatch: add (un)patch hooks Petr Mladek <pmladek@suse.com> - 2017-07-20 18:00 +0200
        Re: [PATCH] livepatch: add (un)patch hooks Josh Poimboeuf <jpoimboe@redhat.com> - 2017-07-20 19:10 +0200
      Re: [PATCH] livepatch: add (un)patch hooks Joe Lawrence <joe.lawrence@redhat.com> - 2017-07-27 22:50 +0200
        Re: [PATCH] livepatch: add (un)patch hooks Josh Poimboeuf <jpoimboe@redhat.com> - 2017-07-27 23:40 +0200
          Re: [PATCH] livepatch: add (un)patch hooks Joe Lawrence <joe.lawrence@redhat.com> - 2017-07-28 20:10 +0200
            Re: [PATCH] livepatch: add (un)patch hooks Josh Poimboeuf <jpoimboe@redhat.com> - 2017-07-28 20:30 +0200

#1692196 — Re: [PATCH] livepatch: add (un)patch hooks

On Mon, Jul 17, 2017 at 05:51:44PM +0200, Petr Mladek wrote:
> On Wed 2017-07-12 10:10:00, Joe Lawrence wrote:
> > When the livepatch core executes klp_(un)patch_object, call out to a
> > livepatch-module specified array of callback hooks.  These hooks provide
> > a notification mechanism for livepatch modules when klp_objects are
> > (un)patching. This may be most interesting when another kernel module
> > is a klp_object target and the livepatch module needs to execute code
> > after the target is loaded, but before its module_init code is run.
> > 
> > The patch-hook executes right before patching objects and the
> > unpatch-hook executes right after unpatching objects.
> > 
> > diff --git a/Documentation/livepatch/hooks.txt b/Documentation/livepatch/hooks.txt
> > new file mode 100644
> > index 000000000000..ef18101a3b90
> > --- /dev/null
> > +++ b/Documentation/livepatch/hooks.txt
> > @@ -0,0 +1,98 @@
> > +(Un)patching Hooks
> > +==================
> > +
> > +Livepatching (un)patch-hooks provide a mechanism to register and execute
> > +a set of callback functions when the kernel's livepatching core performs
> > +an (un)patching operation on a given kernel object.
> 
> The above is correct but it is a bit hard to understand what it really
> means. Josh's discussion about the naming suggests that I am not the only
> one who is confused ;-)
> 
> We need to make it clear that there are 4 basic situations
> where these hooks are called:
> 
>   + patch hook is called when:
> 
> 	1. livepatch is being enabled and object is loaded
> 	2. livepatch is enabled and object is being loaded

These could be stated much simpler:

	1. the object is about to be patched

>   + unpatch hook is called when
> 
> 	3. livepatch is enabled and object is being removed
> 	4. livepatch is being disabled and object is loaded

And this one too:

	2. the object was just unpatched

The rest are just unnecessary details IMO.  When writing a hook for a
particular object, the patch author shouldn't need to care about the
other objects and whether they're patched or unpatched.

Maybe we just need a warning/reminder in the documentation that this
hook is specific to the given object, and other objects could be patched
or unpatched irrespective of the target object's state.

> Note that this document mostly talks only about the two situations
> when the livepatch is enabled and the patched object is being
> loaded or removed.
> 
> But it is still quite tricky to understand what can be modified
> a safe way. We need to be careful about different things
> in the different situations.
> 
> If the patched object is beeing added/removed, we know that its
> code is not being used but the code from the rest of the patch
> is already in use. The module is not yet or not longer properly
> initialized. Therefore it might be too early or too late to
> register or unregister any of its services in the rest of
> the system. Basically it limits the changes only to
> to the object (module) itself.
> 
> If the patch is being enabled, it is another story. The object
> is already initialized and its old code is used but the new
> code from the patch is not yet or not longer used. It suggests
> that it might be safe to do some changes related to the
> new code in the patch. But we need to be careful because
> the system is using the old code.
> 
> 
> But there are actually 4 more situations. If we use the consistency
> model, different parts of the system might use different code.
> I mean that:
> 
>   + patch hook is called also when:
> 
>      + livepatch is being enabled and object is being loaded
>      + livepatch is being disabled and object is being loaded
> 
>   + unpatch hook is called when:
> 
>      + livepatch is being enabled and object is being removed
>      + livepatch is being disabled and object is being removed
> 
> 
> It is a bit easier if you run the hook for vmlinux
> because it is always running.

Again I think this is all overthinking it.  The patch hook should be
specific to the object.  It shouldn't make assumptions about other
objects.

> I am sorry for the long mail. But I have really troubles to
> understand and describe what can be done with these hooks
> a safe way.
> 
> It might help if you share some real-life examples.

Agreed, we should share some real world examples.  For a few cases, load
hooks were extremely useful.  But most of our experience has been with
the kpatch consistency model, so we need to revisit our past findings
and view them through the livepatch lens.

One crazy -- but potentially very useful -- idea would be if the user
were allowed to run stop_machine() from the load hook.  If possible,
that would help prevent a lot of race conditions.

-- 
Josh

[toc] | [next] | [standalone]

#1692395

On Wed, Jul 19, 2017 at 03:49:52PM -0500, Josh Poimboeuf wrote:
> > I am sorry for the long mail. But I have really troubles to
> > understand and describe what can be done with these hooks
> > a safe way.
> > 
> > It might help if you share some real-life examples.
> 
> Agreed, we should share some real world examples.  For a few cases, load
> hooks were extremely useful.  But most of our experience has been with
> the kpatch consistency model, so we need to revisit our past findings
> and view them through the livepatch lens.
> 
> One crazy -- but potentially very useful -- idea would be if the user
> were allowed to run stop_machine() from the load hook.  If possible,
> that would help prevent a lot of race conditions.

To try to give us all a better idea of what's needed, here are some of
the patches that Joe and I looked at before which seem to need load
hooks.  A few of these are ones we actually delivered to customers with
kpatch.  I've tried to re-analyze them in light of the livepatch CM.


First, a few observations:

- Load hooks are a power feature.  They're live patching in "hard mode".
  But they're also very powerful.  Luckily I think they're only needed
  in a few cases, probably < 5% of patches.

- In general, the hooks seem to be useful for cases like:

  - global data updates

  - "patches" to __init and probe functions

  - patching otherwise unpatchable code (i.e., assembly)

  In many/most cases, it seems like stop_machine() would be very useful
  to avoid concurrency issues.

- The more examples I look at, the more I'm thinking we will need both
  pre-patch and post-patch hooks, as well as pre-unpatch and
  post-unpatch hooks.

- The pre-patch and pre-unpatch hooks can be run before the
  patching/unpatching process begins.

- The post-patch and post-unpatch hooks will need to be run from either
  klp_complete_transition() or klp_module_coming/going(), depending on
  whether the to-be-patched module is already loaded or is being
  loaded/unloaded.


Here's a simple example:

  75ff39ccc1bd ("tcp: make challenge acks less predictable")

It involves changing a global sysctl, as well as a patch to the
tcp_send_challenge_ack() function.  We used load hooks to change the
sysctl.

In this case, if we're being super paranoid, it might make sense to
patch the data *after* patching is complete (i.e., a post-patch hook),
so that tcp_send_challenge_ack() could first be changed to read
sysctl_tcp_challenge_ack_limit with READ_ONCE.  But I think the race is
harmless (and such a race already exists in that function with respect
to sysctl writes anyway).

Another way of dealing with concurrency would be to use stop_machine()
in the load hook.


Another example:

  48900cb6af42 ("virtio-net: drop NETIF_F_FRAGLIST")

That changes the net_device features in a driver probe function.  I
don't know exactly how to patch it, but if it's possible, I'm pretty
sure load hooks is the way to do it :-)


Another one:

  54a20552e1ea ("KVM: x86: work around infinite loop in microcode when #AC is delivered")

Again, I have no idea how to do it, but I'd bet that load hooks are
involved.


This one was interesting:

  6f442be2fb22 ("x86_64, traps: Stop using IST for #SS")

A livepatch patch for it is below.  We had something similar for kpatch.
The below patch is completely untested because we don't have
kpatch-build tooling support for livepatch hooks yet.

Note that the load hook would need to run *after* the patch has been
applied and the transition has completed.  And also, it would need to
run inside stop_machine().  I didn't put that in the patch yet.  But it
should at least give you an idea.


diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 819662746e23..68fe9d5f1c22 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -236,6 +236,7 @@ DO_ERROR(X86_TRAP_NP,     SIGBUS,  "segment not present",	segment_not_present)
 #ifdef CONFIG_X86_32
 DO_ERROR(X86_TRAP_SS,     SIGBUS,  "stack segment",		stack_segment)
 #endif
+DO_ERROR(X86_TRAP_SS,     SIGBUS,  "stack segment",		stack_segment_v2)
 DO_ERROR(X86_TRAP_AC,     SIGBUS,  "alignment check",		alignment_check)
 
 #ifdef CONFIG_X86_64
diff --git a/fs/proc/cmdline.c b/fs/proc/cmdline.c
index cbd82dff7e81..504b01dea937 100644
--- a/fs/proc/cmdline.c
+++ b/fs/proc/cmdline.c
@@ -27,3 +27,42 @@ static int __init proc_cmdline_init(void)
 	return 0;
 }
 fs_initcall(proc_cmdline_init);
+
+
+#include "kpatch-macros.h"
+#include <asm/traps.h>
+#include <asm/desc.h>
+#include <asm/cacheflush.h>
+#define trace_stack_segment_v2 stack_segment_v2
+
+static void swapgs_load_hook(void)
+{
+	/* bug doesn't exist on xen */
+	if (paravirt_enabled() && strcmp(pv_info.name, "KVM"))
+		return;
+
+	write_cr0(read_cr0() & ~X86_CR0_WP);
+	barrier();
+
+	/* disable IST for #SS */
+	set_intr_gate(X86_TRAP_SS, stack_segment_v2);
+
+	barrier();
+	write_cr0(read_cr0() | X86_CR0_WP);
+}
+KLP_LOAD_HOOK(swapgs_load_hook);
+
+static void swapgs_unload_hook(void)
+{
+	if (paravirt_enabled() && strcmp(pv_info.name, "KVM"))
+		return;
+
+	write_cr0(read_cr0() & ~X86_CR0_WP);
+	barrier();
+
+	set_intr_gate_ist(X86_TRAP_SS, stack_segment_v2, STACKFAULT_STACK);
+
+	barrier();
+	write_cr0(read_cr0() | X86_CR0_WP);
+}
+KLP_UNLOAD_HOOK(swapgs_unload_hook);

[toc] | [prev] | [next] | [standalone]

#1692399

On Wed, Jul 19, 2017 at 11:17:23PM -0500, Josh Poimboeuf wrote:
> +static void swapgs_unload_hook(void)
> +{
> +	if (paravirt_enabled() && strcmp(pv_info.name, "KVM"))
> +		return;
> +
> +	write_cr0(read_cr0() & ~X86_CR0_WP);
> +	barrier();
> +
> +	set_intr_gate_ist(X86_TRAP_SS, stack_segment_v2, STACKFAULT_STACK);

s/stack_segment_v2/stack_segment/ for the unload hook

> +
> +	barrier();
> +	write_cr0(read_cr0() | X86_CR0_WP);
> +}
> +KLP_UNLOAD_HOOK(swapgs_unload_hook);

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1692407

On Wed, Jul 19, 2017 at 11:17:23PM -0500, Josh Poimboeuf wrote:
> This one was interesting:
> 
>   6f442be2fb22 ("x86_64, traps: Stop using IST for #SS")
> 
> A livepatch patch for it is below.  We had something similar for kpatch.
> The below patch is completely untested because we don't have
> kpatch-build tooling support for livepatch hooks yet.
> 
> Note that the load hook would need to run *after* the patch has been
> applied and the transition has completed.  And also, it would need to
> run inside stop_machine().  I didn't put that in the patch yet.  But it
> should at least give you an idea.

Actually the statement that it needs to run after the patch is applied
isn't accurate.  It was true with an earlier revision of the patch which
modified the do_stack_segment() function.  But this version only adds
code.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1693047

On Wed 2017-07-19 23:17:23, Josh Poimboeuf wrote:
> On Wed, Jul 19, 2017 at 03:49:52PM -0500, Josh Poimboeuf wrote:
> > > I am sorry for the long mail. But I have really troubles to
> > > understand and describe what can be done with these hooks
> > > a safe way.
> > > 
> > > It might help if you share some real-life examples.
> > 
> > Agreed, we should share some real world examples.  For a few cases, load
> > hooks were extremely useful.  But most of our experience has been with
> > the kpatch consistency model, so we need to revisit our past findings
> > and view them through the livepatch lens.
> > 
> > One crazy -- but potentially very useful -- idea would be if the user
> > were allowed to run stop_machine() from the load hook.  If possible,
> > that would help prevent a lot of race conditions.
> 
> To try to give us all a better idea of what's needed, here are some of
> the patches that Joe and I looked at before which seem to need load
> hooks.  A few of these are ones we actually delivered to customers with
> kpatch.  I've tried to re-analyze them in light of the livepatch CM.
> 
> 
> First, a few observations:
> 
> - Load hooks are a power feature.  They're live patching
>   in "hard mode".

I really like this description of the feature!

The original description made me feel that it was something easy
to use and rather safe because the changes were done before
the object was patched or even loaded.

From my point of view, creating a classic livepatch is relatively
easy. The old code is redirected to a fixed one. Where the fixed
one is more or less the same as the new "upstream" fixed code.

Of course, there are some catches, for example, the inline functions,
compiler optimization, semantic changes, functions that cannot be traced.
But all this is kind of discovered and described.

But the hooks allows to do anything. They require writing a custom
code that will never be used anywhere else. It usually has very
limited review if any.

The things gets more complicated when the new code provided by
the livepatch somehow depends on the effect of the hooks and vice
versa. Then the authors must know something about the consistency
models. They might deal with many problems (races) that we were
dealing when implementing the consistency models. And the livepatch
framework does not help much here. In fact, the hooks work in
the immediate mode even when the rest of the patch is applied
using the more careful consistency model.


> - In general, the hooks seem to be useful for cases like:
> 
>   - global data updates
> 
>   - "patches" to __init and probe functions

I think that this is similar to global data updates. The variables
are only harder to find.


>   - patching otherwise unpatchable code (i.e., assembly)
> 
>   In many/most cases, it seems like stop_machine() would be very useful
>   to avoid concurrency issues.

I am not sure if stop_machine() would help here. It would make sense
in kPatch where also the ftrace handlers are added during
stop_machine(). Then it is possible to synchronize both operations
(hooks, enabling ftrace handlers) and do everything "atomically".

IMHO, the big advantage of livepatch framework is that stop_machine()
is not needed. I hope that it will stay this way.

Also it might need some additional support. You would want to stop
the machine to make sure that it is safe to do a change. Then
we might need to check stacks, ...


> - The more examples I look at, the more I'm thinking we will need both
>   pre-patch and post-patch hooks, as well as pre-unpatch and
>   post-unpatch hooks.

Yup.


> - The pre-patch and pre-unpatch hooks can be run before the
>   patching/unpatching process begins.
> 
> - The post-patch and post-unpatch hooks will need to be run from either
>   klp_complete_transition() or klp_module_coming/going(), depending on
>   whether the to-be-patched module is already loaded or is being
>   loaded/unloaded.

Makes sense.

> 
> Here's a simple example:

Thanks a lot for the examples. I have got the idea.

I would formulate it the way that the hooks will allow to
patch/unpatch things that cannot be done by simply using
fixed code instead of the old one.

The use case is:

   + modification of global variables (even more instances)

   + registering newly available services/handlers
     (always in post handlers?)

People need to be careful:

   + synchronize the changes with the rest of the system

   + be aware when the new code from the livepatch is active
     (state of the transition, state of the object(module)


The advantage is that they:

   + allow to enable/disable the changes together with the patch
     (using immediate consistency model)

   + allow to handle both patch enable/disable and object
     load/unload in one place

   + they eventually allow to reject loading the patch
     or the affected object (module) in case of error.


Best Regards,
Petr

[toc] | [prev] | [next] | [standalone]

#1693100

On Thu, Jul 20, 2017 at 05:50:04PM +0200, Petr Mladek wrote:
> On Wed 2017-07-19 23:17:23, Josh Poimboeuf wrote:
> >   - patching otherwise unpatchable code (i.e., assembly)
> > 
> >   In many/most cases, it seems like stop_machine() would be very useful
> >   to avoid concurrency issues.
> 
> I am not sure if stop_machine() would help here. It would make sense
> in kPatch where also the ftrace handlers are added during
> stop_machine(). Then it is possible to synchronize both operations
> (hooks, enabling ftrace handlers) and do everything "atomically".
> 
> IMHO, the big advantage of livepatch framework is that stop_machine()
> is not needed. I hope that it will stay this way.
> 
> Also it might need some additional support. You would want to stop
> the machine to make sure that it is safe to do a change. Then
> we might need to check stacks, ...

Don't worry.  I much prefer our current consistency model to kpatch, and
I have no intention of changing it :-)

That said, for the hooks, I still think stop_machine() will be helpful
in some cases where you need to ensure no other code is running.  Like
the stack_segment patch I posted, for example.

Anyway I'm not suggesting we do the stop_machine() in livepatch code
itself.  I'm just hoping it will work from a hook, in case the patch
author needs to do it as a last resort.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1698305

On 07/20/2017 12:17 AM, Josh Poimboeuf wrote:
> - The pre-patch and pre-unpatch hooks can be run before the
>   patching/unpatching process begins.

Hi Josh,

By "(un)patching process" are you referring to the klp_patch at large or
each klp_object?  ie, would all klp_objects execute their hooks before
anything is (un)patched?  Just trying to clarify.

> - The post-patch and post-unpatch hooks will need to be run from either
>   klp_complete_transition() or klp_module_coming/going(), depending on
>   whether the to-be-patched module is already loaded or is being
>   loaded/unloaded.

You're suggesting that post-(un)patch-hooks:

  1 - Notify klp_objects when a KLP_(UN)PATCHED transition completes

and for subsequently loaded klp_objects (ie modules):

  2 - On load - notify it with current KLP_(UN)PATCHED state,
      Steady state - same as (1) above.

Thanks,

-- Joe

[toc] | [prev] | [next] | [standalone]

#1698336

On Thu, Jul 27, 2017 at 04:43:58PM -0400, Joe Lawrence wrote:
> On 07/20/2017 12:17 AM, Josh Poimboeuf wrote:
> > - The pre-patch and pre-unpatch hooks can be run before the
> >   patching/unpatching process begins.
> 
> Hi Josh,
> 
> By "(un)patching process" are you referring to the klp_patch at large or
> each klp_object?  ie, would all klp_objects execute their hooks before
> anything is (un)patched?  Just trying to clarify.

I think they can be interleaved: call pre-patch hook for object A, patch
object A, call pre-patch hook for object B, patch object B, etc.  We'll
just have to document that each hook is specific to its own object, and
no assumptions can be made about the state of other objects.

So maybe the pre-patch hooks could be called from klp_patch_object() and
pre-unpatch hooks could be called from klp_unpatch_object().  That way
they're called from both patch enable/disable and module coming/going
contexts.

Or maybe it would be cleaner to call the hooks from the *callers* of
klp_patch/unpatch_object().  That would mean a couple of more call
sites, but at least the locations of the pre- hooks would be more
symmetrical with the post- hook locations.  For example,
klp_module_coming() would have both pre- and post- hook calls.

> > - The post-patch and post-unpatch hooks will need to be run from either
> >   klp_complete_transition() or klp_module_coming/going(), depending on
> >   whether the to-be-patched module is already loaded or is being
> >   loaded/unloaded.
> 
> You're suggesting that post-(un)patch-hooks:
> 
>   1 - Notify klp_objects when a KLP_(UN)PATCHED transition completes

Right. (From klp_complete_transition())

> and for subsequently loaded klp_objects (ie modules):
> 
>   2 - On load - notify it with current KLP_(UN)PATCHED state,
>       Steady state - same as (1) above.

Right. (From klp_module_coming/going())

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1698987

On 07/27/2017 05:36 PM, Josh Poimboeuf wrote:
> On Thu, Jul 27, 2017 at 04:43:58PM -0400, Joe Lawrence wrote:
>> On 07/20/2017 12:17 AM, Josh Poimboeuf wrote:
>>> - The post-patch and post-unpatch hooks will need to be run from either
>>>   klp_complete_transition() or klp_module_coming/going(), depending on
>>>   whether the to-be-patched module is already loaded or is being
>>>   loaded/unloaded.
>>
>> You're suggesting that post-(un)patch-hooks:
>>
>>   1 - Notify klp_objects when a KLP_(UN)PATCHED transition completes
> 
> Right. (From klp_complete_transition())

We should be careful to only call hooks for those klp_objects that were
actually (un)patched.  I don't think there is such state that makes it
all the way out to klp_complete_transition(), but since both the
completion and module_coming/going code both operate under the
klp_mutex, perhaps klp_is_object_loaded() is a sufficient check?

>> and for subsequently loaded klp_objects (ie modules):
>>
>>   2 - On load - notify it with current KLP_(UN)PATCHED state,
>>       Steady state - same as (1) above.
> 
> Right. (From klp_module_coming/going())
> 

At least this should be easier to implement since we know what the story
is for the klp_object in hand.

-- Joe

[toc] | [prev] | [next] | [standalone]

#1699002

On Fri, Jul 28, 2017 at 02:08:33PM -0400, Joe Lawrence wrote:
> On 07/27/2017 05:36 PM, Josh Poimboeuf wrote:
> > On Thu, Jul 27, 2017 at 04:43:58PM -0400, Joe Lawrence wrote:
> >> On 07/20/2017 12:17 AM, Josh Poimboeuf wrote:
> >>> - The post-patch and post-unpatch hooks will need to be run from either
> >>>   klp_complete_transition() or klp_module_coming/going(), depending on
> >>>   whether the to-be-patched module is already loaded or is being
> >>>   loaded/unloaded.
> >>
> >> You're suggesting that post-(un)patch-hooks:
> >>
> >>   1 - Notify klp_objects when a KLP_(UN)PATCHED transition completes
> > 
> > Right. (From klp_complete_transition())
> 
> We should be careful to only call hooks for those klp_objects that were
> actually (un)patched.  I don't think there is such state that makes it
> all the way out to klp_complete_transition(), but since both the
> completion and module_coming/going code both operate under the
> klp_mutex, perhaps klp_is_object_loaded() is a sufficient check?

Yeah, the mutex makes it safe to just check klp_is_object_loaded().

-- 
Josh

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web