kprobes incompatibility about kpatch HOT 18 CLOSED

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.

from kpatch.

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

from kpatch.

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

from kpatch.

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.

from kpatch.

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.

from kpatch.

@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.

from kpatch.

@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.

from kpatch.

@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).

from kpatch.

@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.

from kpatch.

@mhiramathitachi Any chance you discussed this with Steven at LinuxCon?

from kpatch.

(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: [email protected]

from kpatch.

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?

from kpatch.

Ah, I see :)
Yes, I'll make a patch and send them to Steven!

from kpatch.

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?

from kpatch.

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.

from kpatch.

@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.

from kpatch.

Hi folks,

Is this still a problem? It works fine in my tests. My test steps are:

1. Use bpftrace to start a ftrace-kprobe;
2. Load live patch for the same function;
3. Verify the bpftrace still works;
4. Verify the function is patched (based on behavior).

from kpatch.

1. Use bpftrace to start a ftrace-kprobe;
2. Load live patch for the same function;
3. Verify the bpftrace still works;
4. 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.

from kpatch.