| Message ID | 20220329104705.65256-1-ammarfaizi2@gnuweeb.org |
|---|---|
| Headers | show |
| Series | Two x86 fixes | expand |
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
Hi, This is the v6 of two x86 fixes. 1) x86/delay: Fix the wrong Assembly constraint in delay_loop() function. 2) x86/MCE/AMD: Fix memory leak when `threshold_create_bank()` fails. ## Changelog v6: - Remove unnecessary Cc tags. - Undo the stable mark for patch 1. - Update commit message, emphasize the danger when the compiler decides to inline the function. - Fix the Fixes tag sha1 in patch 1. - Change the helper function name to __threshold_remove_device(). v5: - Mark patch #1 for stable. - Commit message improvement for patch #1 and #2. - Fold in changes from Yazen and Alviro (for patch #2). v4: - Address comment from Greg, sha1 commit Fixes only needs to be 12 chars. - Add the author of the fixed commit to the CC list. v3: - Fold in changes from Alviro, the previous version is still leaking @bank[n]. v2: - Fix wrong copy/paste. Signed-off-by: Alviro Iskandar Setiawan <alviro.iskandar@gnuweeb.org> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com> Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org> --- Ammar Faizi (2): x86/delay: Fix the wrong asm constraint in `delay_loop()` x86/MCE/AMD: Fix memory leak when `threshold_create_bank()` fails arch/x86/kernel/cpu/mce/amd.c | 32 +++++++++++++++++++------------- arch/x86/lib/delay.c | 4 ++-- 2 files changed, 21 insertions(+), 15 deletions(-) base-commit: 1930a6e739c4b4a654a69164dbe39e554d228915