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

[Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>]

(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