linux-kernel - Re: [PATCH v2 1/2] arm64: implement FTRACE_WITH

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: <20160708114938.49052cee@gandalf.local.home>
Date:	Fri, 8 Jul 2016 11:49:38 -0400
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Petr Mladek <pmladek@...e.com>
Cc:	Torsten Duwe <duwe@....de>,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will.deacon@....com>, takahiro.akashi@...aro.org,
	Jungseok Lee <jungseoklee85@...il.com>,
	Arnd Bergmann <arnd@...db.de>,
	Li Bin <huawei.libin@...wei.com>,
	Ingo Molnar <mingo@...hat.com>,
	Christopher Li <sparse@...isli.org>,
	Jiri Kosina <jikos@...nel.org>, andrew.wafaa@....com,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	live-patching@...r.kernel.org, linux-arch@...r.kernel.org,
	linux-sparse@...r.kernel.org
Subject: Re: [PATCH v2 1/2] arm64: implement FTRACE_WITH_REGS

On Fri, 8 Jul 2016 17:24:21 +0200
Petr Mladek <pmladek@...e.com> wrote:

> On Fri 2016-07-08 17:07:09, Torsten Duwe wrote:
> > On Fri, Jul 08, 2016 at 04:58:00PM +0200, Petr Mladek wrote:  
> > > On Mon 2016-06-27 17:17:17, Torsten Duwe wrote:  
> > > > Once gcc is enhanced to optionally generate NOPs at the beginning
> > > > of each function, like the concept proven in
> > > > https://gcc.gnu.org/ml/gcc-patches/2016-04/msg01671.html
> > > > (sans the "fprintf (... pad_size);", which spoils the data structure
> > > > for kernel use), the generated pads can nicely be used to reroute
> > > > function calls for tracing/profiling, or live patching.
> > > > diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
> > > > index ebecf9a..917065c 100644
> > > > --- a/arch/arm64/kernel/ftrace.c
> > > > +++ b/arch/arm64/kernel/ftrace.c
> > > > @@ -39,6 +39,12 @@ static int ftrace_modify_code(unsigned long pc, u32 old, u32 new,
> > > >  		if (aarch64_insn_read((void *)pc, &replaced))
> > > >  			return -EFAULT;
> > > >  
> > > > +		/* If we already have what we'll finally want,
> > > > +		 * report success. This is needed on startup.
> > > > +		 */
> > > > +		if (replaced == new)
> > > > +			return 0;  
> > > 
> > > This looks strange. I wonder if it actually hides a real bug that we
> > > modify the code twice or so.  
> > 
> > Not at all. All "profilers" we abused so far generate code that needs to
> > be disabled on boot first. prolog-pad generates nops, initially.  
> 
> Yeah, but I cannot find this kind of check in other architectures.
> I checked arch/x86/kernel/ftrace.c, arch/s390/kernel/ftrace.c, and
> arch/powerpc/kernel/ftrace.c. These all support ftrace with
> regs and livepatching.

I guess the question is, with this approach, there's no call to mcount
or fentry at compile time? Just nops are added? In this case perhaps the
if statement should be more defined:

	/*
	 * On boot, with the prologue code, the code will already
	 * be a nop.
	 */
	if (replace == new && new == NOP)
		return 0;

And perhaps you can even pass in addr and check if it equals the nop
address. Maybe even not call this code then? That is, if addr ==
MCOUNT_ADDR passed in by ftrace_code_disable() have ftrace_make_nop()
simple return 0 without doing anything.

-- Steve