Re: livepatch/kprobes incompatibility

[Petr Mladek]

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