On Tue 2016-08-23 21:13:00, Jessica Yu wrote: > Hi Masami, Petr, > > I'm trying to figure out where we are exactly with fixing the problems with > livepatch + kprobes, and I was wondering if there will be any more updates to > the ipmodify patchset that was originally merged back in 2014 (See: > https://lkml.org/lkml/2014/11/20/808). It seems that patch 4/5 ("kprobes: Set > IPMODIFY flag only if the probe can change regs->ip") wasn't merged due to > other ongoing work, and this patch in particular was needed to enforce a hard > conflict between livepatch and jprobes while still enabling livepatch and > kprobes to co-exist. > > Currently, it looks like livepatch/kpatch and kprobes are still in direct > conflict, since both kprobe_ftrace_ops and klp_ops have FTRACE_OPS_FL_IPMODIFY > set. *But* it seems like this mutual exclusion wasn't 100% implemented; I'm > not sure if this was intentional, but kprobes registration will still return > success even when ftrace registration fails due to an ipmodify conflict, and > instead we just get WARNs (See: arm_kprobe_ftrace()). > > So we still end up with buggy situations like the following: > (1) livepatch patches meminfo_proc_show [ succeeds ] > (2) systemtap probes meminfo_proc_show (using kprobes) [ fails ] > * BUT from the user's perspective, it would look like systemtap succeeded, > since register_kprobe() returned success, but the handler will never fire > and only when we look at dmesg do we see that something went wrong > (i.e. ftrace registration had failed since livepatch already reserved > ipmodify in step 1). I tried to improve the error handling of kprobes, see https://lkml.kernel.org/r/1424967232-2923-1-git-send-email-pmladek@xxxxxxx My last notes about this patch set are: + looked again on the error handling of ftrace operations; found that my patches would break optimized kprobes; uff, the kprobes design is not ideal; there are many flags that need to be checked before each operation; it is easy to forget to check one or modify the flags in a wrong order + asked Massami if he would be interested into the 1st patch that was OK; put the rest on hold for a bit > >From what I understand though, there was work being planned to limit this > direct conflict to just livepatch and jprobes, since most of the time kprobes > doesn't change regs->ip. Just wondering what the current state of this work is. My notes about this are: + Jprobe must cause hard conflict because it modifies regs->ip; when the ftrace handlers are finished the code continues with the Jprobe .entry handler; the .entry handlers must end with jprobe_return(). It is quite tricky function because it modifies the stack and calls int3 break. It is handled by a so-called break_handler() from kprobe. It calls post_handler() if any, restores the registry, stack, and goes back to the original function. I am not sure why it works this complicated way. It probably allows to call the .entry handler in a better context, with IRQs enabled? Anyway, the important point is that it modifies regs->ip and forces the ftrace framework to continue with another function. So, it does exactly the same as live patching and therefore they could not work together (at least not in the current state). + kprobe is safe even when it is located on the function+0x0 address. The default kprobe handler does not modify regs->ip; well, in theory kprobe could be used for patching and could do this; + kretprobe is safe as well; the kprobe handler does not modify regs->ip; it just modifies the return address from the function; it does not affect livepatching because the address is defined by the function caller and livepatching keeps it as is Well, there is one more problem. We should also warn when a kprobe is not longer accessible because the function call is redirected by a livepatch. My last notes about it are: + worked on the check for lost Kprobes; decided that only Kprobe knows about all probes and need to be informed about patching; added KPROBE_FLAG_PATCHED and its handling; it will be used by a fake probe that will just signalize that the function is patched; added helper functions that will register and unregister that fake probe; the patchset still needs some clean up before sending Unfortunately, this task has been snowed down in my TODO list and I have not touched it since the spring 2015. I gave it lower priority because we were on the safe side and nobody complained. Best Regards, Petr -- 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