Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932090AbbB0Hfr (ORCPT ); Fri, 27 Feb 2015 02:35:47 -0500 Received: from mail7.hitachi.co.jp ([133.145.228.42]:46264 "EHLO mail7.hitachi.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753279AbbB0Hfq (ORCPT ); Fri, 27 Feb 2015 02:35:46 -0500 Message-ID: <54F01E4A.50001@hitachi.com> Date: Fri, 27 Feb 2015 16:35:38 +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 2/7] kprobes: Propagate error from arm_kprobe_ftrace() References: <1424967232-2923-1-git-send-email-pmladek@suse.cz> <1424967232-2923-3-git-send-email-pmladek@suse.cz> In-Reply-To: <1424967232-2923-3-git-send-email-pmladek@suse.cz> Content-Type: text/plain; charset=ISO-2022-JP Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6342 Lines: 213 (2015/02/27 1:13), Petr Mladek wrote: > arm_kprobe_ftrace() could fail, especially after introducing ftrace IPMODIFY > flag and LifePatching. > > registry_kprobe() and registry_aggr_kprobe() do not mind about the error > because the kprobe gets disabled and they keep it registered. > > But enable_kprobe() should propagate the error because its tasks > fails if ftrace fails. > > Also arm_all_kprobes() should return error if it happens. The behavior > is a bit questionable here. This patch keeps the existing behavior and does > the best effort. It tries to enable as many Kprobes as possible. It returns > only the last error code if any. kprobes_all_disarmed is always cleared and > the message about finished action is always printed. There is going to be > a separate patch that will improve the behavior. When I applied it on -tip/master, there is a hunk which is not cleanly applied. Please rebase it on the latest tip/master, since some logic are changed. Here I have some comments on it. > > Signed-off-by: Petr Mladek > --- > kernel/kprobes.c | 47 ++++++++++++++++++++++++++++++++--------------- > 1 file changed, 32 insertions(+), 15 deletions(-) > > diff --git a/kernel/kprobes.c b/kernel/kprobes.c > index d1b9db690b9c..a69d23add983 100644 > --- a/kernel/kprobes.c > +++ b/kernel/kprobes.c > @@ -929,7 +929,7 @@ static int prepare_kprobe(struct kprobe *p) > } > > /* Caller must lock kprobe_mutex */ > -static void arm_kprobe_ftrace(struct kprobe *p) > +static int arm_kprobe_ftrace(struct kprobe *p) > { > struct kprobe *kp; > int ret; > @@ -949,7 +949,7 @@ static void arm_kprobe_ftrace(struct kprobe *p) > goto err_function; > } > kprobe_ftrace_enabled++; > - return; > + return ret; > > err_function: > ftrace_set_filter_ip(&kprobe_ftrace_ops, (unsigned long)p->addr, 1, 0); > @@ -958,6 +958,7 @@ err_filter: > if (kprobe_aggrprobe(p)) > list_for_each_entry_rcu(kp, &p->list, list) > kp->flags |= KPROBE_FLAG_DISABLED; > + return ret; > } > > /* Caller must lock kprobe_mutex */ > @@ -976,17 +977,15 @@ static void disarm_kprobe_ftrace(struct kprobe *p) > } > #else /* !CONFIG_KPROBES_ON_FTRACE */ > #define prepare_kprobe(p) arch_prepare_kprobe(p) > -#define arm_kprobe_ftrace(p) do {} while (0) > +#define arm_kprobe_ftrace(p) (0) > #define disarm_kprobe_ftrace(p) do {} while (0) > #endif > > /* Arm a kprobe with text_mutex */ > -static void arm_kprobe(struct kprobe *kp) > +static int arm_kprobe(struct kprobe *kp) > { > - if (unlikely(kprobe_ftrace(kp))) { > - arm_kprobe_ftrace(kp); > - return; > - } > + if (unlikely(kprobe_ftrace(kp))) > + return arm_kprobe_ftrace(kp); > /* > * Here, since __arm_kprobe() doesn't use stop_machine(), > * this doesn't cause deadlock on text_mutex. So, we don't > @@ -995,6 +994,7 @@ static void arm_kprobe(struct kprobe *kp) > mutex_lock(&text_mutex); > __arm_kprobe(kp); > mutex_unlock(&text_mutex); > + return 0; > } > > /* Disarm a kprobe with text_mutex */ > @@ -1332,10 +1332,15 @@ out: > put_online_cpus(); > jump_label_unlock(); > > + /* Arm when this is the first enabled kprobe at this address */ > if (ret == 0 && kprobe_disabled(ap) && !kprobe_disabled(p)) { > ap->flags &= ~KPROBE_FLAG_DISABLED; > if (!kprobes_all_disarmed) > - /* Arm the breakpoint again. */ > + /* > + * The kprobe is disabled and warning is printed > + * on error. But we ignore the error code here > + * because we keep it registered. > + */ Why? if we can't arm it, we'd better make it fail. > arm_kprobe(ap); > } > return ret; > @@ -1540,6 +1545,11 @@ int register_kprobe(struct kprobe *p) > &kprobe_table[hash_ptr(p->addr, KPROBE_HASH_BITS)]); > > if (!kprobes_all_disarmed && !kprobe_disabled(p)) > + /* > + * The kprobe is disabled and warning is printed on error. > + * But we ignore the error code here because we keep it > + * registered. > + */ > arm_kprobe(p); Ditto. If we failed to enable it. We should make it fail and report an error to caller. Thank you, > > /* Try to optimize kprobe */ > @@ -2040,7 +2050,7 @@ int enable_kprobe(struct kprobe *kp) > > if (!kprobes_all_disarmed && kprobe_disabled(p)) { > p->flags &= ~KPROBE_FLAG_DISABLED; > - arm_kprobe(p); > + ret = arm_kprobe(p); > } > out: > mutex_unlock(&kprobe_mutex); > @@ -2325,11 +2335,12 @@ static const struct file_operations debugfs_kprobe_blacklist_ops = { > .release = seq_release, > }; > > -static void arm_all_kprobes(void) > +static int arm_all_kprobes(void) > { > struct hlist_head *head; > struct kprobe *p; > unsigned int i; > + int err, ret = 0; > > mutex_lock(&kprobe_mutex); > > @@ -2341,8 +2352,11 @@ static void arm_all_kprobes(void) > for (i = 0; i < KPROBE_TABLE_SIZE; i++) { > head = &kprobe_table[i]; > hlist_for_each_entry_rcu(p, head, hlist) > - if (!kprobe_disabled(p)) > - arm_kprobe(p); > + if (!kprobe_disabled(p)) { > + err = arm_kprobe(p); > + if (err) > + ret = err; > + } > } > > kprobes_all_disarmed = false; > @@ -2350,7 +2364,7 @@ static void arm_all_kprobes(void) > > already_enabled: > mutex_unlock(&kprobe_mutex); > - return; > + return ret; > } > > static void disarm_all_kprobes(void) > @@ -2407,6 +2421,7 @@ static ssize_t write_enabled_file_bool(struct file *file, > { > char buf[32]; > size_t buf_size; > + int err = 0; > > buf_size = min(count, (sizeof(buf)-1)); > if (copy_from_user(buf, user_buf, buf_size)) > @@ -2417,7 +2432,7 @@ static ssize_t write_enabled_file_bool(struct file *file, > case 'y': > case 'Y': > case '1': > - arm_all_kprobes(); > + err = arm_all_kprobes(); > break; > case 'n': > case 'N': > @@ -2428,6 +2443,8 @@ static ssize_t write_enabled_file_bool(struct file *file, > return -EINVAL; > } > > + if (err) > + return err; > return count; > } > > -- Masami HIRAMATSU Software Platform Research Dept. Linux Technology Research Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu.pt@hitachi.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/