Re: [PATCH 0/7] kprobe: Handle error when Kprobe ftrace arming fails

Masami Hiramatsu
2015-03-13T12:36:22Z

(2015/03/13 1:33), Petr Mladek wrote:
> Hi Masami,
>
> On Fri 2015-02-27 16:32:01, Masami Hiramatsu wrote:
>> Hi Petr,
>>
>> (2015/02/27 1:13), Petr Mladek wrote:
>>> arm_kprobe_ftrace() could fail, especially after introducing ftrace IPMODIFY
>>> flag and LifePatching. This patch set adds the error handling and also some
>>> related fixes.
>>
>> Hmm, I'd like to drop IPMODIFY from kprobes except for jprobes,
>> since it actually doesn't change regs->ip which was sent before.
>> It seems that this series partly covers that work.
>>
>>> 1st patch includes the most important change. It helps to keep Kprobes
>>> in a sane state.
>>>
>>> 2nd and 3rd patch allows to propagate the error where needed.
>>
>> OK, I think the 1st one could be merged. 2nd and 3rd one still have some
>> issues as far as I reviewed.
>
> Should I send the 1st patch separately, please?

Yes, please :)

>
> Unfortunately, the 2nd and 3rd patch need much more love. There are
> your comments. I have just realized that they break optimized
> kprobes (the recently fixed issue with kprobes_all_disarmed).
>
> Also I want to think more about the error handling in
> disarm_kprobe_ftrace(). You are right, ftrace should not fail there.
> If it fails, it probably means some inconsistency in the ftrace or kprobes
> structures. My patch expects that the probe is still there. But it is
> more likely that it does not work.
>

Yeah, anyway, we need to improve handling the ftrace failure
in kprobes. Thank you for working on this!

Thanks,

>
>>> The other patches fix problems with the global kprobes_all_disarmed flag.
>>> They were there even before but they become more visible and critical
>>> after the arming errors became propagated.
>>
>> Could you separate the series? And also I doubt we need to show global
>> disable status, since we can check it via debugfs too (and looks
>> redundant).
>
> Yup. In fact, the 6th patch has been obsoleted by the commit
> 69d54b916d83872a ("kprobes: makes kprobes/enabled works correctly for
> optimized kprobes."). 7th is not needed. I want to better understand the
> kprobes code before sending the others again.
>
> Unfortunately, I am often interrupted by other tasks around lifepatching.
>
> Thanks a lot for review.
>
> Best Regards,
> Petr
>
>> Thank you,
>>
>>> The first patch looks rather safe and might be suitable even for 4.0.
>>>
>>> However, I would feel more comfortable if the other patches get some
>>> testing in linux-next. I did quite some testing and did my best. But
>>> I started with the three patches and was surprised by the effect of
>>> the propagated errors. They triggered that BUG_ON() in
>>> __unregister_kprobe_top() are required the other patches
>>> to get it working. I wonder if there is any other scenario that
>>> I have missed.
>>>
>>> Of course, I also wait for feedback how to make things better.
>>>
>>>
>>> Petr Mladek (7):
>>> kprobes: Disable Kprobe when ftrace arming fails
>>> kprobes: Propagate error from arm_kprobe_ftrace()
>>> kprobes: Propagate error from disarm_kprobe_ftrace()
>>> kprobes: Keep consistent state of kprobes_all_disarmed
>>> kprobes: Do not try to disarm already disarmed Kprobe
>>> kprobes: Check kprobes_all_disarmed in kprobe_disarmed()
>>> kprobes: Mark globally disabled Kprobes in debugfs interface
>>>
>>> Documentation/kprobes.txt | 5 +-
>>> kernel/kprobes.c | 279 ++++++++++++++++++++++++++++++++++------------
>>> 2 files changed, 213 insertions(+), 71 deletions(-)
>>>
>>
>>
>> --
>> Masami HIRAMATSU
>> Software Platform Research Dept. Linux Technology Research Center
>> Hitachi, Ltd., Yokohama Research Laboratory
>> E-mail: masami.hiramatsu.pt@hitachi.com
>>
>>
>

--
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com