Skip to contentSkip to conversation

riscv/futex: sign extend compare value in atomic cmpxchg

PATCH Applied Andreas Schwab 6 participants 11 messages Feb 3, 2025 – Feb 14, 2025 RISC-V

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 riscv/futex: sign extend compare value in atomic cmpxchg

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.")
---
 arch/riscv/include/asm/futex.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/riscv/include/asm/futex.h +1 -1
index fc8130f995c1..6907c456ac8c 100644
@@ -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();
 
Signature · 7 lines
2.48.1


-- 
Andreas Schwab, SUSE Labs, [email protected]
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Reviewed-by on v1 1/1
#1 lore raw
Quoted 7 lines · Andreas Schwab

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.")

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!

Let's add a link to Jessica's comment as well:

Quoted 30 lines
---
 arch/riscv/include/asm/futex.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/riscv/include/asm/futex.h +1 -1
index fc8130f995c1..6907c456ac8c 100644
@@ -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();
 
Signature · 12 lines
2.48.1


-- 
Andreas Schwab, SUSE Labs, [email protected]
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv
#2 lore raw
Quoted 11 lines · Björn Töpel
Quoted 7 lines · Andreas Schwab

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.")

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.

Signature · 3 lines
Andreas Schwab, SUSE Labs, [email protected]
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
#3 lore raw
In reply to Andreas Schwab
Quoted 5 lines · Andreas Schwab

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

Quoted 32 lines
Fixes b90edb33010b ("RISC-V: Add futex support.")

---
arch/riscv/include/asm/futex.h | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)
arch/riscv/include/asm/futex.h +1 -1
index fc8130f995c1..6907c456ac8c 100644
@@ -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();
 
Signature · 12 lines
2.48.1


-- 
Andreas Schwab, SUSE Labs, [email protected]
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv
#4 lore raw
Quoted 8 lines · Jessica Clarke
Quoted 5 lines · Andreas Schwab

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,

It doesn't, that's the point!

Signature · 3 lines
Andreas Schwab, SUSE Labs, [email protected]
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
reviewer Reviewed-by on v1 1/1
#5 lore raw
In reply to Andreas Schwab

Hi Andreas,

Quoted 16 lines · Andreas Schwab

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.")
---
  arch/riscv/include/asm/futex.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
arch/riscv/include/asm/futex.h +0 -0
index fc8130f995c1..6907c456ac8c 100644

arch/riscv/include/asm/futex.h futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr, · line 99

Quoted 8 lines
arch/riscv/include/asm/futex.h +1 -1
@@ -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();
 

Thanks,

Alex

#6 lore raw
In reply to Andreas Schwab
This patch was applied to riscv/linux.git (fixes) by Palmer Dabbelt <[email protected]>:

Hello:

This patch was applied to riscv/linux.git (fixes)
by Palmer Dabbelt <[email protected]>:

Quoted 9 lines · On Mon, 03 Feb 2025 11:06:00 +0100 you 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.")

[...]

Here is the summary with links:
- riscv/futex: sign extend compare value in atomic cmpxchg

    https://git.kernel.org/riscv/c/5c238584bce5

You are awesome, thank you!

Signature · 2 lines
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
#7 lore raw

Hi,

Quoted 17 lines · On Fri, Feb 14, 2025 at 2:31 AM <[email protected]> wrote:

Hello:

This patch was applied to riscv/linux.git (fixes)
by Palmer Dabbelt <[email protected]>:

Quoted 9 lines · On Mon, 03 Feb 2025 11:06:00 +0100 you 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.")

[...]

Quoted 10 lines

Here is the summary with links:
- riscv/futex: sign extend compare value in atomic cmpxchg

    https://git.kernel.org/riscv/c/5c238584bce5

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

I applied this patch, but it doesn't seem to take effect. There is no sign extension for a2 in the assembly code. What did I miss? GCC version >= 13.

ffffffff800e87e0 <futex_atomic_cmpxchg_inatomic>:
ffffffff800e87e0: 1141 addi sp,sp,-16
ffffffff800e87e2: e422 sd s0,8(sp)
ffffffff800e87e4: 2bf01793 bseti a5,zero,0x3f
ffffffff800e87e8: 0800 addi s0,sp,16
ffffffff800e87ea: 17ed addi a5,a5,-5
ffffffff800e87ec: 00b7f663 bgeu a5,a1,ffffffff800e87f8
<futex_atomic_cmpxchg_inatomic+0x18>
ffffffff800e87f0: 6422 ld s0,8(sp)
ffffffff800e87f2: 5549 li a0,-14
ffffffff800e87f4: 0141 addi sp,sp,16
ffffffff800e87f6: 8082 ret
ffffffff800e87f8: 872a mv a4,a0
ffffffff800e87fa: 00040837 lui a6,0x40
ffffffff800e87fe: 10082073 csrs sstatus,a6
ffffffff800e8802: 4781 li a5,0
ffffffff800e8804: 1605a8af lr.w.aqrl a7,(a1)
ffffffff800e8808: 00c89563 bne a7,a2,ffffffff800e8812
<futex_atomic_cmpxchg_inatomic+0x32>
ffffffff800e880c: 1ed5a52f sc.w.aqrl a0,a3,(a1)
ffffffff800e8810: f975 bnez a0,ffffffff800e8804
<futex_atomic_cmpxchg_inatomic+0x24>
ffffffff800e8812: 0007851b sext.w a0,a5
ffffffff800e8816: 10083073 csrc sstatus,a6
ffffffff800e881a: 01172023 sw a7,0(a4)
ffffffff800e881e: 6422 ld s0,8(sp)
ffffffff800e8820: 0141 addi sp,sp,16
ffffffff800e8822: 8082 ret

Should we do it like this:
__asm__ __volatile__ (
" sext.w %[ov], %[ov] \n"
...

Thanks,
Yunhui

#8 lore raw
Quoted 56 lines · yunhui cui

Hi,

Quoted 17 lines · On Fri, Feb 14, 2025 at 2:31 AM <[email protected]> wrote:

Hello:

This patch was applied to riscv/linux.git (fixes)
by Palmer Dabbelt <[email protected]>:

Quoted 9 lines · On Mon, 03 Feb 2025 11:06:00 +0100 you 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.")

[...]

Quoted 10 lines

Here is the summary with links:
- riscv/futex: sign extend compare value in atomic cmpxchg
https://git.kernel.org/riscv/c/5c238584bce5

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

I applied this patch, but it doesn't seem to take effect. There is no sign extension for a2 in the assembly code. What did I miss? GCC version >= 13.

ffffffff800e87e0 <futex_atomic_cmpxchg_inatomic>:
ffffffff800e87e0: 1141 addi sp,sp,-16
ffffffff800e87e2: e422 sd s0,8(sp)
ffffffff800e87e4: 2bf01793 bseti a5,zero,0x3f
ffffffff800e87e8: 0800 addi s0,sp,16
ffffffff800e87ea: 17ed addi a5,a5,-5
ffffffff800e87ec: 00b7f663 bgeu a5,a1,ffffffff800e87f8
<futex_atomic_cmpxchg_inatomic+0x18>
ffffffff800e87f0: 6422 ld s0,8(sp)
ffffffff800e87f2: 5549 li a0,-14
ffffffff800e87f4: 0141 addi sp,sp,16
ffffffff800e87f6: 8082 ret
ffffffff800e87f8: 872a mv a4,a0
ffffffff800e87fa: 00040837 lui a6,0x40
ffffffff800e87fe: 10082073 csrs sstatus,a6
ffffffff800e8802: 4781 li a5,0
ffffffff800e8804: 1605a8af lr.w.aqrl a7,(a1)
ffffffff800e8808: 00c89563 bne a7,a2,ffffffff800e8812
<futex_atomic_cmpxchg_inatomic+0x32>
ffffffff800e880c: 1ed5a52f sc.w.aqrl a0,a3,(a1)

Quoted 8 lines

ffffffff800e8810: f975 bnez a0,ffffffff800e8804
<futex_atomic_cmpxchg_inatomic+0x24>
ffffffff800e8812: 0007851b sext.w a0,a5
ffffffff800e8816: 10083073 csrc sstatus,a6
ffffffff800e881a: 01172023 sw a7,0(a4)
ffffffff800e881e: 6422 ld s0,8(sp)
ffffffff800e8820: 0141 addi sp,sp,16
ffffffff800e8822: 8082 ret

The calling convention means a2 will be sign-extended on entry to the function, and since your compiler has not done anything to change the value that will still be true. So it has (legitimately) optimised out any sign extension as a no-op. Are you seeing any problems that you believe to be caused by this function, or are you just inspecting the disassembly to satisfy your own curiosity?

Quoted 4 lines

Should we do it like this:
__asm__ __volatile__ (
" sext.w %[ov], %[ov] \n"
...

No, it’s unnecessary and prevents optimisation.

Jess

Quoted 7 lines

Thanks,
Yunhui

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv

#9 lore raw

Hi Jess,

Quoted 67 lines · Jessica Clarke
Quoted 56 lines · yunhui cui

Hi,

Quoted 17 lines · On Fri, Feb 14, 2025 at 2:31 AM <[email protected]> wrote:

Hello:

This patch was applied to riscv/linux.git (fixes)
by Palmer Dabbelt <[email protected]>:

Quoted 9 lines · On Mon, 03 Feb 2025 11:06:00 +0100 you 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.")

[...]

Quoted 10 lines

Here is the summary with links:
- riscv/futex: sign extend compare value in atomic cmpxchg
https://git.kernel.org/riscv/c/5c238584bce5

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

I applied this patch, but it doesn't seem to take effect. There is no sign extension for a2 in the assembly code. What did I miss? GCC version >= 13.

ffffffff800e87e0 <futex_atomic_cmpxchg_inatomic>:
ffffffff800e87e0: 1141 addi sp,sp,-16
ffffffff800e87e2: e422 sd s0,8(sp)
ffffffff800e87e4: 2bf01793 bseti a5,zero,0x3f
ffffffff800e87e8: 0800 addi s0,sp,16
ffffffff800e87ea: 17ed addi a5,a5,-5
ffffffff800e87ec: 00b7f663 bgeu a5,a1,ffffffff800e87f8
<futex_atomic_cmpxchg_inatomic+0x18>
ffffffff800e87f0: 6422 ld s0,8(sp)
ffffffff800e87f2: 5549 li a0,-14
ffffffff800e87f4: 0141 addi sp,sp,16
ffffffff800e87f6: 8082 ret
ffffffff800e87f8: 872a mv a4,a0
ffffffff800e87fa: 00040837 lui a6,0x40
ffffffff800e87fe: 10082073 csrs sstatus,a6
ffffffff800e8802: 4781 li a5,0
ffffffff800e8804: 1605a8af lr.w.aqrl a7,(a1)
ffffffff800e8808: 00c89563 bne a7,a2,ffffffff800e8812
<futex_atomic_cmpxchg_inatomic+0x32>
ffffffff800e880c: 1ed5a52f sc.w.aqrl a0,a3,(a1)

Quoted 8 lines

ffffffff800e8810: f975 bnez a0,ffffffff800e8804
<futex_atomic_cmpxchg_inatomic+0x24>
ffffffff800e8812: 0007851b sext.w a0,a5
ffffffff800e8816: 10083073 csrc sstatus,a6
ffffffff800e881a: 01172023 sw a7,0(a4)
ffffffff800e881e: 6422 ld s0,8(sp)
ffffffff800e8820: 0141 addi sp,sp,16
ffffffff800e8822: 8082 ret

Quoted 4 lines

The calling convention means a2 will be sign-extended on entry to the function, and since your compiler has not done anything to change the value that will still be true. So it has (legitimately) optimised out any sign extension as a no-op.

If so, why this patch?

Quoted 3 lines

Are you seeing any problems that you
believe to be caused by this function, or are you just inspecting the disassembly to satisfy your own curiosity?

No actual problem traced here.

Quoted 18 lines
Quoted 5 lines

Should we do it like this:
__asm__ __volatile__ (
" sext.w %[ov], %[ov] \n"
...

No, it’s unnecessary and prevents optimisation.

Jess

Quoted 7 lines

Thanks,
Yunhui

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv

Thanks,
Yunhui

#10 lore raw
Quoted 71 lines · yunhui cui

Hi Jess,

Quoted 67 lines · Jessica Clarke
Quoted 64 lines · yunhui cui

Hi,

Quoted 17 lines · On Fri, Feb 14, 2025 at 2:31 AM <[email protected]> wrote:

Hello:

This patch was applied to riscv/linux.git (fixes)
by Palmer Dabbelt <[email protected]>:

Quoted 9 lines · On Mon, 03 Feb 2025 11:06:00 +0100 you 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.")

[...]

Quoted 10 lines

Here is the summary with links:
- riscv/futex: sign extend compare value in atomic cmpxchg
https://git.kernel.org/riscv/c/5c238584bce5

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

I applied this patch, but it doesn't seem to take effect. There is no sign extension for a2 in the assembly code. What did I miss? GCC version >= 13.

ffffffff800e87e0 <futex_atomic_cmpxchg_inatomic>:
ffffffff800e87e0: 1141 addi sp,sp,-16
ffffffff800e87e2: e422 sd s0,8(sp)
ffffffff800e87e4: 2bf01793 bseti a5,zero,0x3f
ffffffff800e87e8: 0800 addi s0,sp,16
ffffffff800e87ea: 17ed addi a5,a5,-5
ffffffff800e87ec: 00b7f663 bgeu a5,a1,ffffffff800e87f8
<futex_atomic_cmpxchg_inatomic+0x18>
ffffffff800e87f0: 6422 ld s0,8(sp)
ffffffff800e87f2: 5549 li a0,-14
ffffffff800e87f4: 0141 addi sp,sp,16
ffffffff800e87f6: 8082 ret
ffffffff800e87f8: 872a mv a4,a0
ffffffff800e87fa: 00040837 lui a6,0x40
ffffffff800e87fe: 10082073 csrs sstatus,a6
ffffffff800e8802: 4781 li a5,0
ffffffff800e8804: 1605a8af lr.w.aqrl a7,(a1)
ffffffff800e8808: 00c89563 bne a7,a2,ffffffff800e8812
<futex_atomic_cmpxchg_inatomic+0x32>
ffffffff800e880c: 1ed5a52f sc.w.aqrl a0,a3,(a1)
ffffffff800e8810: f975 bnez a0,ffffffff800e8804
<futex_atomic_cmpxchg_inatomic+0x24>
ffffffff800e8812: 0007851b sext.w a0,a5
ffffffff800e8816: 10083073 csrc sstatus,a6
ffffffff800e881a: 01172023 sw a7,0(a4)
ffffffff800e881e: 6422 ld s0,8(sp)
ffffffff800e8820: 0141 addi sp,sp,16
ffffffff800e8822: 8082 ret

Quoted 6 lines
Quoted 4 lines

The calling convention means a2 will be sign-extended on entry to the function, and since your compiler has not done anything to change the value that will still be true. So it has (legitimately) optimised out any sign extension as a no-op.

If so, why this patch?

Because there are multiple ways in which compilers can emit code that does not keep values sign-extended in registers within a function. The calling convention only provides that guarantee on entry to a function following the standard calling convention. In the specific instance of compiling the version of Linux you are compiling with the kernel config you are using and the precise compiler you are using, the patch happens to not be needed for this snippet of the binary because the generated code already has the sign-extended value in the register used. But just because there are instances when the patch does nothing doesn’t mean all instances are like that.

Jess

Quoted 28 lines
Quoted 3 lines

Are you seeing any problems that you
believe to be caused by this function, or are you just inspecting the disassembly to satisfy your own curiosity?

No actual problem traced here.

Quoted 18 lines
Quoted 5 lines

Should we do it like this:
__asm__ __volatile__ (
" sext.w %[ov], %[ov] \n"
...

No, it’s unnecessary and prevents optimisation.

Jess

Quoted 7 lines

Thanks,
Yunhui

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv

Thanks,
Yunhui