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
| ||
|
Date: Mon, 01 Jul 2019 14:21:17 +0530 From: "Naveen N. Rao" <naveen.n.rao@...ux.vnet.ibm.com> To: Steven Rostedt <rostedt@...dmis.org> 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 Steven Rostedt wrote: > 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. > <snip> >> >> 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> Thanks for confirming! We do need an IPI to be sure, as you pointed out above. I will have the patching out take the same path to simplify things. - Naveen
Powered by blists - more mailing lists