From mboxrd@z Thu Jan 1 00:00:00 1970 From: Masami Hiramatsu Subject: Re: [PATCH v3 4/7] kprobes: Replace %p with other pointer types Date: Sat, 28 Apr 2018 00:42:02 +0900 Message-ID: <20180428004202.4297e781304b5eda53100f67@kernel.org> References: <152481117776.22588.1210388093668905564.stgit@devbox> <152481130062.22588.10747314456340608852.stgit@devbox> <20180427065636.ells635z7cw4b4oz@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20180427065636.ells635z7cw4b4oz@gmail.com> Sender: stable-owner@vger.kernel.org To: Ingo Molnar Cc: linux-kernel@vger.kernel.org, linux-arch@vger.kernel.org, Ingo Molnar , "H . Peter Anvin" , x86@kernel.org, Ananth N Mavinakayanahalli , Anil S Keshavamurthy , "David S . Miller" , Jon Medhurst , Will Deacon , Arnd Bergmann , David Howells , Heiko Carstens , "Tobin C . Harding" , Linus Torvalds , Thomas Richter , akpm@linux-foundation.org, acme@kernel.org, rostedt@goodmis.org, brueckner@linux.vnet.ibm.com, schwidefsky@de.ibm.com, stable@vger.kernel.org List-Id: linux-arch.vger.kernel.org On Fri, 27 Apr 2018 08:56:36 +0200 Ingo Molnar wrote: > > * Masami Hiramatsu wrote: > > > @@ -712,7 +712,7 @@ static void reuse_unused_kprobe(struct kprobe *ap) > > op = container_of(ap, struct optimized_kprobe, kp); > > if (unlikely(list_empty(&op->list))) > > printk(KERN_WARNING "Warning: found a stray unused " > > - "aggrprobe@%p\n", ap->addr); > > + "aggrprobe@%pS\n", ap->addr); > > Is this a kernel bug? If yes then please convert this to a WARN_ON_ONCE() that > doesn't print any kernel addresses. OK, I will do. > > > /* Enable the probe again */ > > ap->flags &= ~KPROBE_FLAG_DISABLED; > > /* Optimize it again (remove from op->list) */ > > @@ -985,7 +985,8 @@ static int arm_kprobe_ftrace(struct kprobe *p) > > ret = ftrace_set_filter_ip(&kprobe_ftrace_ops, > > (unsigned long)p->addr, 0, 0); > > if (ret) { > > - pr_debug("Failed to arm kprobe-ftrace at %p (%d)\n", p->addr, ret); > > + pr_debug("Failed to arm kprobe-ftrace at %pS (%d)\n", > > + p->addr, ret); > > return ret; > > Can this happen during normal use? If yes then just remove the warning message. Yes, it can, for example probing on live-patched function. > > > } > > > > @@ -1025,7 +1026,8 @@ static int disarm_kprobe_ftrace(struct kprobe *p) > > > > ret = ftrace_set_filter_ip(&kprobe_ftrace_ops, > > (unsigned long)p->addr, 1, 0); > > - WARN(ret < 0, "Failed to disarm kprobe-ftrace at %p (%d)\n", p->addr, ret); > > + WARN(ret < 0, "Failed to disarm kprobe-ftrace at %pS (%d)\n", > > + p->addr, ret); > > As this is a signal of a kernel bug, just convert this to a regular WARN_ONCE() > that doesn't print any kernel addresses. Would you mean just replace WARN with WARN_ONCE as below? WARN_ONCE(ret < 0, "Failed to disarm kprobe-ftrace at %pS (%d)\n", p->addr, ret); Anyway, without printing kprobe->addr, it may be meaningless for this case. > > > +/* Caller must NOT call this in usual path. This is only for critical case */ > > void dump_kprobe(struct kprobe *kp) > > { > > - printk(KERN_WARNING "Dumping kprobe:\n"); > > - printk(KERN_WARNING "Name: %s\nAddress: %p\nOffset: %x\n", > > - kp->symbol_name, kp->addr, kp->offset); > > + pr_err("Dumping kprobe:\n"); > > + pr_err("Name: %s\nOffset: %x\nAddress: %pS\n", > > + kp->symbol_name, kp->offset, kp->addr); > > No, this function should just go away and be replaced by a WARN() in > reenter_kprobe(). Would you consider to use pr_err() here? If so, I'll move this dump as you suggested. Thank you, -- Masami Hiramatsu From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.kernel.org ([198.145.29.99]:48362 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757457AbeD0PmH (ORCPT ); Fri, 27 Apr 2018 11:42:07 -0400 Date: Sat, 28 Apr 2018 00:42:02 +0900 From: Masami Hiramatsu Subject: Re: [PATCH v3 4/7] kprobes: Replace %p with other pointer types Message-ID: <20180428004202.4297e781304b5eda53100f67@kernel.org> In-Reply-To: <20180427065636.ells635z7cw4b4oz@gmail.com> References: <152481117776.22588.1210388093668905564.stgit@devbox> <152481130062.22588.10747314456340608852.stgit@devbox> <20180427065636.ells635z7cw4b4oz@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-arch-owner@vger.kernel.org List-ID: To: Ingo Molnar Cc: linux-kernel@vger.kernel.org, linux-arch@vger.kernel.org, Ingo Molnar , "H . Peter Anvin" , x86@kernel.org, Ananth N Mavinakayanahalli , Anil S Keshavamurthy , "David S . Miller" , Jon Medhurst , Will Deacon , Arnd Bergmann , David Howells , Heiko Carstens , "Tobin C . Harding" , Linus Torvalds , Thomas Richter , akpm@linux-foundation.org, acme@kernel.org, rostedt@goodmis.org, brueckner@linux.vnet.ibm.com, schwidefsky@de.ibm.com, stable@vger.kernel.org Message-ID: <20180427154202.6AKBh1eKp5VDKztpe5_rHSPZ48-xPeyHWZiabwGHrrM@z> On Fri, 27 Apr 2018 08:56:36 +0200 Ingo Molnar wrote: > > * Masami Hiramatsu wrote: > > > @@ -712,7 +712,7 @@ static void reuse_unused_kprobe(struct kprobe *ap) > > op = container_of(ap, struct optimized_kprobe, kp); > > if (unlikely(list_empty(&op->list))) > > printk(KERN_WARNING "Warning: found a stray unused " > > - "aggrprobe@%p\n", ap->addr); > > + "aggrprobe@%pS\n", ap->addr); > > Is this a kernel bug? If yes then please convert this to a WARN_ON_ONCE() that > doesn't print any kernel addresses. OK, I will do. > > > /* Enable the probe again */ > > ap->flags &= ~KPROBE_FLAG_DISABLED; > > /* Optimize it again (remove from op->list) */ > > @@ -985,7 +985,8 @@ static int arm_kprobe_ftrace(struct kprobe *p) > > ret = ftrace_set_filter_ip(&kprobe_ftrace_ops, > > (unsigned long)p->addr, 0, 0); > > if (ret) { > > - pr_debug("Failed to arm kprobe-ftrace at %p (%d)\n", p->addr, ret); > > + pr_debug("Failed to arm kprobe-ftrace at %pS (%d)\n", > > + p->addr, ret); > > return ret; > > Can this happen during normal use? If yes then just remove the warning message. Yes, it can, for example probing on live-patched function. > > > } > > > > @@ -1025,7 +1026,8 @@ static int disarm_kprobe_ftrace(struct kprobe *p) > > > > ret = ftrace_set_filter_ip(&kprobe_ftrace_ops, > > (unsigned long)p->addr, 1, 0); > > - WARN(ret < 0, "Failed to disarm kprobe-ftrace at %p (%d)\n", p->addr, ret); > > + WARN(ret < 0, "Failed to disarm kprobe-ftrace at %pS (%d)\n", > > + p->addr, ret); > > As this is a signal of a kernel bug, just convert this to a regular WARN_ONCE() > that doesn't print any kernel addresses. Would you mean just replace WARN with WARN_ONCE as below? WARN_ONCE(ret < 0, "Failed to disarm kprobe-ftrace at %pS (%d)\n", p->addr, ret); Anyway, without printing kprobe->addr, it may be meaningless for this case. > > > +/* Caller must NOT call this in usual path. This is only for critical case */ > > void dump_kprobe(struct kprobe *kp) > > { > > - printk(KERN_WARNING "Dumping kprobe:\n"); > > - printk(KERN_WARNING "Name: %s\nAddress: %p\nOffset: %x\n", > > - kp->symbol_name, kp->addr, kp->offset); > > + pr_err("Dumping kprobe:\n"); > > + pr_err("Name: %s\nOffset: %x\nAddress: %pS\n", > > + kp->symbol_name, kp->offset, kp->addr); > > No, this function should just go away and be replaced by a WARN() in > reenter_kprobe(). Would you consider to use pr_err() here? If so, I'll move this dump as you suggested. Thank you, -- Masami Hiramatsu