| Message ID | 20220329104705.65256-2-ammarfaizi2@gnuweeb.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Two x86 fixes | expand |
On 3/29/22 03:47, Ammar Faizi wrote: > The asm constraint does not reflect that the asm statement can modify > the value of @loops. But the asm statement in delay_loop() does modify > the @loops. > > Specifiying the wrong constraint may lead to undefined behavior, it may > clobber random stuff (e.g. local variable, important temporary value in > regs, etc.). This is especially dangerous when the compiler decides to > inline the function and since it doesn't know that the value gets > modified, it might decide to use it from a register directly without > reloading it. > > Fix this by changing the constraint from "a" (as an input) to "+a" (as > an input and output). Was this found by inspection or was it causing real-world problems?
On 4/2/22 12:42 AM, Dave Hansen wrote:
> Was this found by inspection or was it causing real-world problems?
It was found by inspection, no real-world problems found so far.
On Tue, Mar 29 2022 at 17:47, Ammar Faizi wrote: > The asm constraint does not reflect that the asm statement can modify > the value of @loops. But the asm statement in delay_loop() does modify > the @loops. > > Specifiying the wrong constraint may lead to undefined behavior, it may > clobber random stuff (e.g. local variable, important temporary value in > regs, etc.). This is especially dangerous when the compiler decides to > inline the function and since it doesn't know that the value gets > modified, it might decide to use it from a register directly without > reloading it. > > Fix this by changing the constraint from "a" (as an input) to "+a" (as > an input and output). This analysis is plain wrong. The assembly code operates on a register and not on memory: asm volatile( " test %0,%0 \n" " jz 3f \n" " jmp 1f \n" ".align 16 \n" "1: jmp 2f \n" ".align 16 \n" "2: dec %0 \n" " jnz 2b \n" "3: dec %0 \n" : /* we don't need output */ ----> :"a" (loops) This tells the compiler to use [RE]AX and initialize it from the variable 'loops'. It's never written back because all '%0' in the above assembly are substituted with [RE]AX. This also tells the compiler that the inline assembly clobbers [RE]AX and that's all it needs to know. Nothing to fix here, whether the code is inlined or not. Thanks, tglx
On 4/3/22 11:57 PM, Thomas Gleixner wrote: > On Tue, Mar 29 2022 at 17:47, Ammar Faizi wrote: >> The asm constraint does not reflect that the asm statement can modify >> the value of @loops. But the asm statement in delay_loop() does modify >> the @loops. >> >> Specifiying the wrong constraint may lead to undefined behavior, it may >> clobber random stuff (e.g. local variable, important temporary value in >> regs, etc.). This is especially dangerous when the compiler decides to >> inline the function and since it doesn't know that the value gets >> modified, it might decide to use it from a register directly without >> reloading it. >> >> Fix this by changing the constraint from "a" (as an input) to "+a" (as >> an input and output). > > This analysis is plain wrong. The assembly code operates on a register > and not on memory: > asm volatile( > " test %0,%0 \n" > " jz 3f \n" > " jmp 1f \n" > > ".align 16 \n" > "1: jmp 2f \n" > > ".align 16 \n" > "2: dec %0 \n" > " jnz 2b \n" > "3: dec %0 \n" > > : /* we don't need output */ > ----> :"a" (loops) > > This tells the compiler to use [RE]AX and initialize it from the > variable 'loops'. It's never written back because all '%0' in the above > assembly are substituted with [RE]AX. This also tells the compiler that > the inline assembly clobbers [RE]AX and that's all it needs to know. Hi Thomas, Thanks for taking a look. I doubt about your sentence "This also tells the compiler that the inline assembly clobbers [RE]AX". How come it tells the compiler that the inline ASM clobbers [RE]AX? That's an input constraint. Doesn't that mean it is read-only for the ASM statement? That means the compiler is allowed to assume [RE]AX doesn't change inside the ASM statement. Those `dec`s do really change the [RE]AX. Please review this again. Thanks!
On Sun, Apr 03 2022 at 18:57, Thomas Gleixner wrote: > On Tue, Mar 29 2022 at 17:47, Ammar Faizi wrote: >> The asm constraint does not reflect that the asm statement can modify >> the value of @loops. But the asm statement in delay_loop() does modify >> the @loops. >> >> Specifiying the wrong constraint may lead to undefined behavior, it may >> clobber random stuff (e.g. local variable, important temporary value in >> regs, etc.). This is especially dangerous when the compiler decides to >> inline the function and since it doesn't know that the value gets >> modified, it might decide to use it from a register directly without >> reloading it. Ignore me, I misread this part of the explanation. Thanks, tglx
diff --git a/arch/x86/lib/delay.c b/arch/x86/lib/delay.c index 65d15df6212d..0e65d00e2339 100644 --- a/arch/x86/lib/delay.c +++ b/arch/x86/lib/delay.c @@ -54,8 +54,8 @@ static void delay_loop(u64 __loops) " jnz 2b \n" "3: dec %0 \n" - : /* we don't need output */ - :"a" (loops) + : "+a" (loops) + : ); }
The asm constraint does not reflect that the asm statement can modify the value of @loops. But the asm statement in delay_loop() does modify the @loops. Specifiying the wrong constraint may lead to undefined behavior, it may clobber random stuff (e.g. local variable, important temporary value in regs, etc.). This is especially dangerous when the compiler decides to inline the function and since it doesn't know that the value gets modified, it might decide to use it from a register directly without reloading it. Fix this by changing the constraint from "a" (as an input) to "+a" (as an input and output). Cc: David Laight <David.Laight@ACULAB.COM> Cc: Jiri Hladky <hladky.jiri@googlemail.com> Cc: Alviro Iskandar Setiawan <alviro.iskandar@gnuweeb.org> Fixes: e01b70ef3eb3 ("x86: fix bug in arch/i386/lib/delay.c file, delay_loop function") Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org> --- v6: - Remove unnecessary Cc tags. - Update commit message, emphasize the danger when the compiler decides to inline the function. - Fix the Fixes tag sha1. - Remove stable Cc. --- arch/x86/lib/delay.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)