[lkml]   [2015]   [Mar]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH 0/7] kprobe: Handle error when Kprobe ftrace arming fails
(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!


>>> 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(-)
>> --
>> Software Platform Research Dept. Linux Technology Research Center
>> Hitachi, Ltd., Yokohama Research Laboratory
>> E-mail:

Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory

 \ /
  Last update: 2015-03-13 13:41    [W:0.061 / U:21.952 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site