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