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

[Petr Mladek]

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?

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.


> > 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@xxxxxxxxxxx
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/