From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754930AbbCMMgW (ORCPT ); Fri, 13 Mar 2015 08:36:22 -0400 Received: from mail7.hitachi.co.jp ([133.145.228.42]:45165 "EHLO mail7.hitachi.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752465AbbCMMgU (ORCPT ); Fri, 13 Mar 2015 08:36:20 -0400 Message-ID: <5502D9BD.9030405@hitachi.com> Date: Fri, 13 Mar 2015 21:36:13 +0900 From: Masami Hiramatsu Organization: Hitachi, Ltd., Japan User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20120614 Thunderbird/13.0.1 MIME-Version: 1.0 To: Petr Mladek CC: "David S. Miller" , Anil S Keshavamurthy , Ananth N Mavinakayanahalli , Frederic Weisbecker , Ingo Molnar , Steven Rostedt , Jiri Kosina , linux-kernel@vger.kernel.org Subject: Re: [PATCH 0/7] kprobe: Handle error when Kprobe ftrace arming fails References: <1424967232-2923-1-git-send-email-pmladek@suse.cz> <54F01D71.1030303@hitachi.com> <20150312163354.GF1849@dhcp128.suse.cz> In-Reply-To: <20150312163354.GF1849@dhcp128.suse.cz> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org (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