linux-kernel - Re: [PATCH v3 4/7] kprobes: Replace %p with other pointer types

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20180428004202.4297e781304b5eda53100f67@kernel.org>
Date:   Sat, 28 Apr 2018 00:42:02 +0900
From:   Masami Hiramatsu <mhiramat@...nel.org>
To:     Ingo Molnar <mingo@...nel.org>
Cc:     linux-kernel@...r.kernel.org, linux-arch@...r.kernel.org,
        Ingo Molnar <mingo@...hat.com>,
        "H . Peter Anvin" <hpa@...or.com>, x86@...nel.org,
        Ananth N Mavinakayanahalli <ananth@...ibm.com>,
        Anil S Keshavamurthy <anil.s.keshavamurthy@...el.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jon Medhurst <tixy@...aro.org>,
        Will Deacon <will.deacon@....com>,
        Arnd Bergmann <arnd@...db.de>,
        David Howells <dhowells@...hat.com>,
        Heiko Carstens <heiko.carstens@...ibm.com>,
        "Tobin C . Harding" <me@...in.cc>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Thomas Richter <tmricht@...ux.ibm.com>,
        akpm@...ux-foundation.org, acme@...nel.org, rostedt@...dmis.org,
        brueckner@...ux.vnet.ibm.com, schwidefsky@...ibm.com,
        stable@...r.kernel.org
Subject: Re: [PATCH v3 4/7] kprobes: Replace %p with other pointer types

On Fri, 27 Apr 2018 08:56:36 +0200
Ingo Molnar <mingo@...nel.org> wrote:

> 
> * Masami Hiramatsu <mhiramat@...nel.org> 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 <mhiramat@...nel.org>