| Message ID | mvmfrkv2vhz.fsf@suse.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | riscv/futex: sign extend compare value in atomic cmpxchg | expand |
| Context | Check | Description |
|---|---|---|
| bjorn/pre-ci_am | success | Success |
| bjorn/build-rv32-defconfig | success | build-rv32-defconfig |
| bjorn/build-rv64-clang-allmodconfig | success | build-rv64-clang-allmodconfig |
| bjorn/build-rv64-gcc-allmodconfig | success | build-rv64-gcc-allmodconfig |
| bjorn/build-rv64-nommu-k210-defconfig | success | build-rv64-nommu-k210-defconfig |
| bjorn/build-rv64-nommu-k210-virt | success | build-rv64-nommu-k210-virt |
| bjorn/checkpatch | success | checkpatch |
| bjorn/dtb-warn-rv64 | success | dtb-warn-rv64 |
| bjorn/header-inline | success | header-inline |
| bjorn/kdoc | success | kdoc |
| bjorn/module-param | success | module-param |
| bjorn/verify-fixes | success | verify-fixes |
| bjorn/verify-signedoff | success | verify-signedoff |
Andreas Schwab <schwab@suse.de> writes: > Make sure the compare value in the lr/sc loop is sign extended to match > what lr.w does. Fortunately, due to the compiler keeping the register > contents sign extended anyway the lack of the explicit extension didn't > result in wrong code so far, but this cannot be relied upon. > > Fixes: b90edb33010b ("RISC-V: Add futex support.") > Signed-off-by: Andreas Schwab <schwab@suse.de> Hmm, in this scenario we *can* rely on it, no (inline vs macro)? Regardless, having an explicit cast there doesn't hurt, and make it more obvious! Reviewed-by: Björn Töpel <bjorn@rivosinc.com> Let's add a link to Jessica's comment as well: Link: https://lore.kernel.org/linux-riscv/CC2D9220-F8DE-4CC8-ACAD-7B1A21E276FE@jrtc27.com/ > --- > arch/riscv/include/asm/futex.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/riscv/include/asm/futex.h b/arch/riscv/include/asm/futex.h > index fc8130f995c1..6907c456ac8c 100644 > --- a/arch/riscv/include/asm/futex.h > +++ b/arch/riscv/include/asm/futex.h > @@ -93,7 +93,7 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr, > _ASM_EXTABLE_UACCESS_ERR(1b, 3b, %[r]) \ > _ASM_EXTABLE_UACCESS_ERR(2b, 3b, %[r]) \ > : [r] "+r" (ret), [v] "=&r" (val), [u] "+m" (*uaddr), [t] "=&r" (tmp) > - : [ov] "Jr" (oldval), [nv] "Jr" (newval) > + : [ov] "Jr" ((long)(int)oldval), [nv] "Jr" (newval) > : "memory"); > __disable_user_access(); > > -- > 2.48.1 > > > -- > Andreas Schwab, SUSE Labs, schwab@suse.de > GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 > "And now for something completely different." > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
On Feb 03 2025, Björn Töpel wrote: > Andreas Schwab <schwab@suse.de> writes: > >> Make sure the compare value in the lr/sc loop is sign extended to match >> what lr.w does. Fortunately, due to the compiler keeping the register >> contents sign extended anyway the lack of the explicit extension didn't >> result in wrong code so far, but this cannot be relied upon. >> >> Fixes: b90edb33010b ("RISC-V: Add futex support.") >> Signed-off-by: Andreas Schwab <schwab@suse.de> > > Hmm, in this scenario we *can* rely on it, no (inline vs macro)? No, the issue is that the asm operand (oldval) is u32, but the asm is using the 64-bit value from the register. You cannot expect that the compiler keeps the upper half defined in any way at this point. That is different for the operand that is passed in from newval, because sc.w is only using the low 32-bits from the operand.
On 3 Feb 2025, at 10:06, Andreas Schwab <schwab@suse.de> wrote: > > Make sure the compare value in the lr/sc loop is sign extended to match > what lr.w does. Fortunately, due to the compiler keeping the register > contents sign extended anyway the lack of the explicit extension didn't > result in wrong code so far, but this cannot be relied upon. GCC may guarantee this today, but LLVM does not, and definitely generates code that does not do so. Whether that happens for any of the consumers of this API today I don’t know, but it is definitely relying on things that aren’t true for LLVM, and are not specified in the psABI. Jess > Fixes: b90edb33010b ("RISC-V: Add futex support.") > Signed-off-by: Andreas Schwab <schwab@suse.de> > --- > arch/riscv/include/asm/futex.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/riscv/include/asm/futex.h b/arch/riscv/include/asm/futex.h > index fc8130f995c1..6907c456ac8c 100644 > --- a/arch/riscv/include/asm/futex.h > +++ b/arch/riscv/include/asm/futex.h > @@ -93,7 +93,7 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr, > _ASM_EXTABLE_UACCESS_ERR(1b, 3b, %[r]) \ > _ASM_EXTABLE_UACCESS_ERR(2b, 3b, %[r]) \ > : [r] "+r" (ret), [v] "=&r" (val), [u] "+m" (*uaddr), [t] "=&r" (tmp) > - : [ov] "Jr" (oldval), [nv] "Jr" (newval) > + : [ov] "Jr" ((long)(int)oldval), [nv] "Jr" (newval) > : "memory"); > __disable_user_access(); > > -- > 2.48.1 > > > -- > Andreas Schwab, SUSE Labs, schwab@suse.de > GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 > "And now for something completely different." > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
Hi Andreas, On 03/02/2025 11:06, Andreas Schwab wrote: > Make sure the compare value in the lr/sc loop is sign extended to match > what lr.w does. Fortunately, due to the compiler keeping the register > contents sign extended anyway the lack of the explicit extension didn't > result in wrong code so far, but this cannot be relied upon. > > Fixes: b90edb33010b ("RISC-V: Add futex support.") > Signed-off-by: Andreas Schwab <schwab@suse.de> > --- > arch/riscv/include/asm/futex.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/riscv/include/asm/futex.h b/arch/riscv/include/asm/futex.h > index fc8130f995c1..6907c456ac8c 100644 > --- a/arch/riscv/include/asm/futex.h > +++ b/arch/riscv/include/asm/futex.h > @@ -93,7 +93,7 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr, > _ASM_EXTABLE_UACCESS_ERR(1b, 3b, %[r]) \ > _ASM_EXTABLE_UACCESS_ERR(2b, 3b, %[r]) \ > : [r] "+r" (ret), [v] "=&r" (val), [u] "+m" (*uaddr), [t] "=&r" (tmp) > - : [ov] "Jr" (oldval), [nv] "Jr" (newval) > + : [ov] "Jr" ((long)(int)oldval), [nv] "Jr" (newval) > : "memory"); > __disable_user_access(); > Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com> Thanks, Alex
diff --git a/arch/riscv/include/asm/futex.h b/arch/riscv/include/asm/futex.h index fc8130f995c1..6907c456ac8c 100644 --- a/arch/riscv/include/asm/futex.h +++ b/arch/riscv/include/asm/futex.h @@ -93,7 +93,7 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr, _ASM_EXTABLE_UACCESS_ERR(1b, 3b, %[r]) \ _ASM_EXTABLE_UACCESS_ERR(2b, 3b, %[r]) \ : [r] "+r" (ret), [v] "=&r" (val), [u] "+m" (*uaddr), [t] "=&r" (tmp) - : [ov] "Jr" (oldval), [nv] "Jr" (newval) + : [ov] "Jr" ((long)(int)oldval), [nv] "Jr" (newval) : "memory"); __disable_user_access();
Make sure the compare value in the lr/sc loop is sign extended to match what lr.w does. Fortunately, due to the compiler keeping the register contents sign extended anyway the lack of the explicit extension didn't result in wrong code so far, but this cannot be relied upon. Fixes: b90edb33010b ("RISC-V: Add futex support.") Signed-off-by: Andreas Schwab <schwab@suse.de> --- arch/riscv/include/asm/futex.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)