powerpc: Ensure gcc doesn't move around cache flushing in __patch_instruction

Message ID 48284701fe497bb4f5bede5c55bbce9d70309562.camel@kernel.crashing.org
State New
Headers show
Series
  • powerpc: Ensure gcc doesn't move around cache flushing in __patch_instruction
Related show

Commit Message

Benjamin Herrenschmidt May 17, 2018, 3:06 a.m.
The current asm statement in __patch_instruction() for the cache flushes
doesn't have a "volatile" statement and no memory clobber. That means
gcc can potentially move it around (or move the store done by put_user
past the flush).

Add both to ensure gcc doesn't play games.

Found by code inspection, no actual bug reported.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---

Comments

Segher Boessenkool May 17, 2018, 7:23 p.m. | #1
Hi!

On Thu, May 17, 2018 at 01:06:10PM +1000, Benjamin Herrenschmidt wrote:
> The current asm statement in __patch_instruction() for the cache flushes
> doesn't have a "volatile" statement and no memory clobber. That means
> gcc can potentially move it around (or move the store done by put_user
> past the flush).

volatile is completely superfluous here, except maybe as documentation:
any asm without outputs is always volatile.

(And the memory clobber does not prevent the compiler from moving the
asm around, or duplicating it, etc., and neither does the volatile).


Segher
Benjamin Herrenschmidt May 17, 2018, 10:30 p.m. | #2
On Thu, 2018-05-17 at 14:23 -0500, Segher Boessenkool wrote:
> Hi!
> 
> On Thu, May 17, 2018 at 01:06:10PM +1000, Benjamin Herrenschmidt wrote:
> > The current asm statement in __patch_instruction() for the cache flushes
> > doesn't have a "volatile" statement and no memory clobber. That means
> > gcc can potentially move it around (or move the store done by put_user
> > past the flush).
> 
> volatile is completely superfluous here, except maybe as documentation:
> any asm without outputs is always volatile.

I wasn't aware of that. I was drilled early on to always stick volatile
in my asm statements if they have any form of side effect :-)

> (And the memory clobber does not prevent the compiler from moving the
> asm around, or duplicating it, etc., and neither does the volatile).

It prevents load/stores from moving around doesn't it ? I wanted to
make sure the store of the instruction doesn't move in/pass the asm. If
you say that's not needed then ignore the patch.

Cheers,
Ben.
 
> 
> Segher
Segher Boessenkool May 17, 2018, 11 p.m. | #3
On Fri, May 18, 2018 at 08:30:27AM +1000, Benjamin Herrenschmidt wrote:
> On Thu, 2018-05-17 at 14:23 -0500, Segher Boessenkool wrote:
> > On Thu, May 17, 2018 at 01:06:10PM +1000, Benjamin Herrenschmidt wrote:
> > > The current asm statement in __patch_instruction() for the cache flushes
> > > doesn't have a "volatile" statement and no memory clobber. That means
> > > gcc can potentially move it around (or move the store done by put_user
> > > past the flush).
> > 
> > volatile is completely superfluous here, except maybe as documentation:
> > any asm without outputs is always volatile.
> 
> I wasn't aware of that. I was drilled early on to always stick volatile
> in my asm statements if they have any form of side effect :-)

If an asm without output was not marked automatically as having another
side effect, every such asm would be immediately deleted ;-)

Adding volatile as documentation for side effects can be good; it just
doesn't do much (nothing, in fact) for asms without output as far as
the compiler is concerned.

> > (And the memory clobber does not prevent the compiler from moving the
> > asm around, or duplicating it, etc., and neither does the volatile).
> 
> It prevents load/stores from moving around doesn't it ? I wanted to
> make sure the store of the instruction doesn't move in/pass the asm. If
> you say that's not needed then ignore the patch.

No, it's fine here, and you want either that or put exactly the memory
you are touching in a constraint (probably overkill here).  I just
wanted to say that a "memory" clobber does nothing more than say the
asm touches some unspecified memory; there is no magic other meaning
to it.  Your patch is correct, just the "volatile" part isn't needed,
and the explanation was a bit cargo-culty ;-)


Segher

Patch

--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c
@@ -32,8 +32,9 @@  static int __patch_instruction(unsigned int *exec_addr, unsigned int instr,
        if (err)
                return err;
 
-       asm ("dcbst 0, %0; sync; icbi 0,%1; sync; isync" :: "r" (patch_addr),
-                                                           "r" (exec_addr));
+       asm volatile("dcbst 0, %0; sync; icbi 0,%1; sync; isync"
+                    :: "r" (patch_addr), "r" (exec_addr)
+                    : "memory");
 
        return 0;
 }