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

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

[Benjamin Herrenschmidt]

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

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

[Segher Boessenkool]

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

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

[Benjamin Herrenschmidt]

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

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

[Segher Boessenkool]

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

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

[Christophe Leroy]

Le 18/05/2018 à 01:00, Segher Boessenkool a écrit :
> 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 ;-)
> 

Any plan to get that merged ?

Christophe