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

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>
---

--- 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;
 }

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

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:

quoted

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

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:

quoted

On Thu, May 17, 2018 at 01:06:10PM +1000, Benjamin Herrenschmidt wrote:

quoted

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.

quoted

(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

Le 18/05/2018 à 01:00, Segher Boessenkool a écrit :

On Fri, May 18, 2018 at 08:30:27AM +1000, Benjamin Herrenschmidt wrote:

quoted

On Thu, 2018-05-17 at 14:23 -0500, Segher Boessenkool wrote:

quoted

On Thu, May 17, 2018 at 01:06:10PM +1000, Benjamin Herrenschmidt wrote:

quoted

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.

quoted

quoted

(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 ;-)

Any plan to get that merged ?

Christophe