linux-kernel - Re: [PATCH v2 4/7] powerpc/ftrace: Additionally nop out the preceding mflr with -mprofile-kernel

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20190627121344.25b5449a@gandalf.local.home>
Date:   Thu, 27 Jun 2019 12:13:44 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     "Naveen N. Rao" <naveen.n.rao@...ux.vnet.ibm.com>
Cc:     linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Ingo Molnar <mingo@...nel.org>,
        Michael Ellerman <mpe@...erman.id.au>,
        Nicholas Piggin <npiggin@...il.com>
Subject: Re: [PATCH v2 4/7] powerpc/ftrace: Additionally nop out the
 preceding mflr with -mprofile-kernel

On Thu, 27 Jun 2019 20:58:20 +0530
"Naveen N. Rao" <naveen.n.rao@...ux.vnet.ibm.com> wrote:

> 
> > But interesting, I don't see a synchronize_rcu_tasks() call
> > there.  
> 
> We felt we don't need it in this case. We patch the branch to ftrace 
> with a nop first. Other cpus should see that first. But, now that I 
> think about it, should we add a memory barrier to ensure the writes get 
> ordered properly?

Do you send an ipi to the other CPUs. I would just to be safe.

> >> -	if (patch_instruction((unsigned int *)ip, pop)) {
> >> +	/*
> >> +	 * Our original call site looks like:
> >> +	 *
> >> +	 * bl <tramp>
> >> +	 * ld r2,XX(r1)
> >> +	 *
> >> +	 * Milton Miller pointed out that we can not simply nop the branch.
> >> +	 * If a task was preempted when calling a trace function, the nops
> >> +	 * will remove the way to restore the TOC in r2 and the r2 TOC will
> >> +	 * get corrupted.
> >> +	 *
> >> +	 * Use a b +8 to jump over the load.
> >> +	 */
> >> +	if (patch_instruction((unsigned int *)ip, PPC_INST_BRANCH | 8)) {
> >>  		pr_err("Patching NOP failed.\n");
> >>  		return -EPERM;
> >>  	}
> >> +#endif /* CONFIG_MPROFILE_KERNEL */
> >>  
> >>  	return 0;
> >>  }
> >> @@ -421,6 +429,26 @@ static int __ftrace_make_nop_kernel(struct dyn_ftrace *rec, unsigned long addr)
> >>  		return -EPERM;
> >>  	}
> >>  
> >> +#ifdef CONFIG_MPROFILE_KERNEL  
> > 
> > I would think you need to break this up into two parts as well, with a
> > synchronize_rcu_tasks() in between.
> > 
> > Imagine this scenario:
> > 
> > 	<func>:
> > 	nop <-- interrupt comes here, and preempts the task
> > 	nop
> > 
> > First change.
> > 
> > 	<func>:
> > 	mflr	r0
> > 	nop
> > 
> > Second change.
> > 
> > 	<func>:
> > 	mflr	r0
> > 	bl	_mcount
> > 
> > Task returns from interrupt
> > 
> > 	<func>:
> > 	mflr	r0
> > 	bl	_mcount <-- executes here
> > 
> > It never did the mflr r0, because the last command that was executed
> > was a nop before it was interrupted. And yes, it can be interrupted
> > on a nop!  
> 
> We are handling this through ftrace_replace_code() and 
> __ftrace_make_call_prep() below. For FTRACE_UPDATE_MAKE_CALL, we patch 
> in the mflr, followed by smp_call_function(isync) and 
> synchronize_rcu_tasks() before we proceed to patch the branch to ftrace.
> 
> I don't see any other scenario where we end up in 
> __ftrace_make_nop_kernel() without going through ftrace_replace_code().  
> For kernel modules, this can happen during module load/init and so, I 
> patch out both instructions in __ftrace_make_call() above without any 
> synchronization.
> 
> Am I missing anything?
> 

No, I think I got confused ;-), it's the patch out that I was worried
about, but when I was going through the scenario, I somehow turned it
into the patching in (which I already audited :-p). I was going to
reply with just the top part of this email, but then the confusion
started :-/

OK, yes, patching out should be fine, and you already covered the
patching in. Sorry for the noise.

Just to confirm and totally remove the confusion, the patch does:

	<func>:
	mflr	r0 <-- preempt here
	bl	_mcount

	<func>:
	mflr	r0
	nop

And this is fine regardless.

OK, Reviewed-by: Steven Rostedt (VMware) <rostedt@...dmis.org>

-- Steve