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

[Josh Poimboeuf]

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);