livepatch/kprobes incompatibility

livepatch/kprobes incompatibility

[Jessica Yu]

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

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

Thanks,
Jessica

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

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

Re: livepatch/kprobes incompatibility

[Masami Hiramatsu]

On Tue, 23 Aug 2016 21:13:00 -0400
Jessica Yu <jeyu@redhat.com> 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.

Hmm, it seems I have missed to follow it up.
I'll try refresh it for the latest kernel again.

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

Right, jprobes and livepatch can not work together, but kprobe should be
available.

Thanks,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: livepatch/kprobes incompatibility

[Masami Hiramatsu]

On Wed, 24 Aug 2016 11:39:59 +0200
Petr Mladek <pmladek@suse.com> wrote:

> 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

I think the warning is acceptable but no need to update by flag,
since the livepatch can be removed and kprobes can come back again.
Moreover, at the lower level like kprobes, user might want to use
it for monitoring that the patched code is actually not executed,
like security inspection.

Thank you,


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: livepatch/kprobes incompatibility

[Josh Poimboeuf]

On Wed, Aug 24, 2016 at 11:39:59AM +0200, Petr Mladek wrote:
>   + 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

BTW, I think there's still a kretprobe issue which affects the accuracy
of the unwinder and the consistency model.  The unwinder will report
kretprobe_trampoline instead of the real caller's return address.

It can probably be fixed similarly to how ftrace function_graph tracing
does it with a ftrace_graph_ret_addr() helper:

  https://lkml.kernel.org/r/cover.1471607358.git.jpoimboe@redhat.com

-- 
Josh