kprobes incompatibility
jpoimboe opened this issue · 18 comments
When kprobes registers with ftrace before kpatch does for the same function, kprobes "wins", until the kprobe has been unregistered with ftrace. The kernel kprobe_ftrace_handler function changes regs->ip, which overwrites the value which kpatch had written.
Option 1:
diff --git a/arch/x86/kernel/kprobes/ftrace.c b/arch/x86/kernel/kprobes/ftrace.c
index 23ef5c5..5969176 100644
--- a/arch/x86/kernel/kprobes/ftrace.c
+++ b/arch/x86/kernel/kprobes/ftrace.c
@@ -57,6 +57,7 @@ void __kprobes kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
struct kprobe *p;
struct kprobe_ctlblk *kcb;
unsigned long flags;
+ unsigned long save_ip = regs->ip;
/* Disable irq for emulating a breakpoint and avoiding preempt */
local_irq_save(flags);
@@ -80,6 +81,8 @@ void __kprobes kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
* If pre_handler returns !0, it sets regs->ip and
* resets current kprobe.
*/
+ if (ip != save_ip)
+ regs->ip = save_ip;
}
end:
local_irq_restore(flags);
With this patch, if both kpatch and a kprobe handler try to modify the IP, kpatch always wins.
Option 2:
Create an FTRACE_OPS_FL_LAST flag which ensures that kpatch is called last, so that any kprobes attempts to update regs->ip will be ignored.
This is another variation on option 1: if both kpatch and a kprobe handler try to modify the IP, kpatch always wins.
Option 3:
Create an FTRACE_OPS_FL_FIRST flag which ensures that kpatch is called first. Then pass the updated regs->ip to the kprobes functions so they can set their breakpoint at the beginning of the replacement function. The downside here is that kprobes modules can override kpatch's redirecting of functions, but that may not be much of an issue.
So if both kpatch and a kprobe handler try to modify the IP, kprobes wins.
Option 4:
Create an FTRACE_OPS_FL_IPMODIFY flag: first to register wins.
Recreate procedure:
First start systemtap to enable the kprobe:
~ $ sudo stap -v -e 'probe kernel.function("meminfo_proc_show") {printf("meminfo_proc_show called\n");}'
Pass 1: parsed user script and 104 library script(s) using 218480virt/35716res/3064shr/33300data kb, in 110usr/10sys/114real ms.
Pass 2: analyzed script: 1 probe(s), 0 function(s), 0 embed(s), 0 global(s) using 409876virt/173104res/105960shr/67656data kb, in 480usr/60sys/542real ms.
Pass 3: using cached /root/.systemtap/cache/9f/stap_9f022595f0a4c768e04c75362b24228b_975.c
Pass 4: using cached /root/.systemtap/cache/9f/stap_9f022595f0a4c768e04c75362b24228b_975.ko
Pass 5: starting run.
Then in another window:
~ $ cat meminfo-string.patch
Index: src/fs/proc/meminfo.c
===================================================================
--- src.orig/fs/proc/meminfo.c
+++ src/fs/proc/meminfo.c
@@ -95,7 +95,7 @@ static int meminfo_proc_show(struct seq_
"Committed_AS: %8lu kB\n"
"VmallocTotal: %8lu kB\n"
"VmallocUsed: %8lu kB\n"
- "VmallocChunk: %8lu kB\n"
+ "VMALLOCCHUNK: %8lu kB\n"
#ifdef CONFIG_MEMORY_FAILURE
"HardwareCorrupted: %5lu kB\n"
#endif
~ $ kpatch build meminfo-string.patch
Using cache at /home/jpoimboe/.kpatch/3.13.5-202.fc20.x86_64/src
Building original kernel
Building patched kernel
Detecting changed objects
Rebuilding changed objects
Extracting new and modified ELF sections
Building patch module: kpatch-meminfo-string.ko
SUCCESS
~ $ grep -i chunk /proc/meminfo
VmallocChunk: 34359330256 kB
~ $ sudo insmod kpatch-meminfo-string.ko
~ $ grep -i chunk /proc/meminfo
VmallocChunk: 34359330256 kB
Notice the patch doesn't work. Now kill the stap process and the patch takes effect:
~ $ grep -i chunk /proc/meminfo
VMALLOCCHUNK: 34359330256 kB
I had a good discussion about this with Masami Hiramatsu (kprobes maintainer). In my opinion we should go with option 4 because I think kpatching a kprobed function is an obscure and confusing use case that isn't worth supporting. Option 4 requires a new kprobes interface, but it resolves multiple issues (3 by my count).
Option 4 still isn't perfect though. You wouldn't be able to kpatch a kprobed function, but you could still kprobe a kpatched function, which could cause a little bit of confusion if the kprobes were on the original version of the function.
Also, we must consider the probing case after patching. We should introduce FTRACE_OPS_FL_IPMODIFY and if there is already a handler which has the flag at the given address, ftrace should reject new one.
This makes the solution easier. Kprobes and kpatch don't need to consider each other directly, but just get an error when using ftrace.
@mhiramat Yes, if some kprobes handlers modify IP, then I think an FTRACE_OPS_FL_IPMODIFY flag makes sense. One question: how will kprobes know if a given handler will modify the IP? Or would it always set the flag regardless?
@rostedt had also suggested a FTRACE_OPS_FL_PERMANENT flag, so that a patch wouldn't ever get removed, even if function_trace_stop gets set.
@jpoimboe all kprobes handlers is possible to change IP and it also requires the original (unmodified) IP address since it gets a kprobe handler from that. So anyway all kprobes must set the FTRACE_OPS_FL_IPMODIFY. to ensure ftrace passes unmodified IP.
@mhiramat I talked with Steven about this a little bit. The problem with kprobes always setting FTRACE_OPS_FL_IPMODIFY is that it makes kprobes and kpatch always incompatible. If kprobes doesn't change regs->ip (which is probably true 99% of the time) then they should be able to co-exist.
Another problem is that ftrace can't enforce it. If somebody doesn't provide FTRACE_OPS_FL_IPMODIFY, it would be hard for ftrace to verify that they aren't changing regs->ip.
So how about this:
- If a kprobe handler needs to modify IP, user sets KPROBE_FLAG_IPMODIFY flag to register_kprobe, and then kprobes sets FTRACE_OPS_FL_IPMODIFY when registering with ftrace for that probe.
- if KPROBE_FLAG_IPMODIFY is not used, kprobe_ftrace_handler() can detect when a kprobe handler changes regs->ip and restore it to its original value (regs->ip = ip).
@jpoimboe I think your suggestion is OK for me. Most of the case, jprobe is the only kprobe user which can modify IP, and jprobe itself is rarely used.
@mhiramathitachi Any chance you discussed this with Steven at LinuxCon?
(2014/05/28 12:18), Josh Poimboeuf wrote:
@mhiramathitachi Any chance you discussed this with Steven at LinuxCon?
Ah, I've heard that Steven will not be there. Last week I met him at LinuxCon Japan
and talked about kpatch and others.
Anyway I'll fix this issue on upstream kernel asap, since only kprobes is the actual
user on current upstream kernel.
Thank you, :)
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com
Sorry, I meant to say LinuxCon Japan :-) Are you planning on doing something like KPROBE_FLAG_IPMODIFY and FTRACE_OPS_FL_IPMODIFY like described above?
Ah, I see :)
Yes, I'll make a patch and send them to Steven!
Housekeeping: is there anything left to do that the introduction of the FTRACE_OPS_FL_IPMODIFY doesn't fix? That was added way back in Nov 2014 with f8b8be8a310a ftrace, kprobes: Support IPMODIFY flag to find IP modify conflict.
I still see kprobe_ftrace_ops() always setting FTRACE_OPS_FL_IPMODIFY so maybe @jpoimboe's suggestions are still TODO?
Wow this is an old one. @mhiramathitachi @mhiramat are you still there? :-)
The original suggestion was
- If a kprobe handler needs to modify IP, user sets KPROBE_FLAG_IPMODIFY flag to register_kprobe, and then kprobes sets FTRACE_OPS_FL_IPMODIFY when registering with ftrace for that probe.
- if KPROBE_FLAG_IPMODIFY is not used, kprobe_ftrace_handler() can detect when a kprobe handler changes regs->ip and restore it to its original value (regs->ip = ip).
I don't think this has been much of a problem. Masami, do you have any plans to do that? If not, we can close this.
@jpoimboe, Thank you for correcting my account.:-)
Hmm, I have to look it carefully, since I and bpf guys already introduced generic fault-injection using kprobes.
Hi folks,
Is this still a problem? It works fine in my tests. My test steps are:
- Use bpftrace to start a ftrace-kprobe;
- Load live patch for the same function;
- Verify the bpftrace still works;
- Verify the function is patched (based on behavior).
- Use bpftrace to start a ftrace-kprobe;
- Load live patch for the same function;
- Verify the bpftrace still works;
- Verify the function is patched (based on behavior).
Also, when I unload the live patch in 2, the bpftrace in 1 continues to work as expected.
Hi @liu-song-6 : thanks for checking, Masami fixed this in the kernel a while back with [PATCH] kprobes: Allow kprobes coexist with livepatch, so we can close this long-open issue.