| Message ID | 20240728133223.885378-1-dilfridge@gentoo.org |
|---|---|
| State | New |
| Headers | show |
| Series | Mitigation for "clone on sparc might fail with -EFAULT for no valid reason" (bz 31394) | expand |
It seems the kernel can not deal with uncommitted stack space in the area intended
for the register window when executing the clone() system call. So create a nested
frame (proxy for the kernel frame) and flush it from the processor to memory to
force committing pages to the stack before invoking the system call.
Signed-off-by: Michael Karcher<sourceware-bugzilla@mkarcher.dialup.fu-berlin.de>
Bug: https://www.mail-archive.com/debian-glibc@lists.debian.org/msg62592.html
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31394
See-also: https://lore.kernel.org/sparclinux/62f9be9d-a086-4134-9a9f-5df8822708af@mkarcher.dialup.fu-berlin.de/
---
sysdeps/unix/sysv/linux/sparc/sparc32/clone.S | 3 +++
sysdeps/unix/sysv/linux/sparc/sparc64/clone.S | 3 +++
2 files changed, 6 insertions(+)
diff --git a/sysdeps/unix/sysv/linux/sparc/sparc32/clone.S b/sysdeps/unix/sysv/linux/sparc/sparc32/clone.S
index 748d25fcfe..c9cf9bb055 100644
--- a/sysdeps/unix/sysv/linux/sparc/sparc32/clone.S
+++ b/sysdeps/unix/sysv/linux/sparc/sparc32/clone.S
@@ -28,6 +28,9 @@
.text
ENTRY (__clone)
save %sp,-96,%sp
+ save %sp,-96,%sp
+ flushw
+ restore
cfi_def_cfa_register(%fp)
cfi_window_save
cfi_register(%o7, %i7)
diff --git a/sysdeps/unix/sysv/linux/sparc/sparc64/clone.S b/sysdeps/unix/sysv/linux/sparc/sparc64/clone.S
index e5ff2cf1a0..370d51fda2 100644
--- a/sysdeps/unix/sysv/linux/sparc/sparc64/clone.S
+++ b/sysdeps/unix/sysv/linux/sparc/sparc64/clone.S
@@ -32,6 +32,9 @@
ENTRY (__clone)
save %sp, -192, %sp
+ save %sp, -192, %sp
+ flushw
+ restore
cfi_def_cfa_register(%fp)
cfi_window_save
cfi_register(%o7, %i7)
* Andreas K. Hüttel: > From: Michael Karcher <sourceware-bugzilla@mkarcher.dialup.fu-berlin.de> > > Bug: https://www.mail-archive.com/debian-glibc@lists.debian.org/msg62592.html > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31394 > See-also: https://lore.kernel.org/sparclinux/62f9be9d-a086-4134-9a9f-5df8822708af@mkarcher.dialup.fu-berlin.de/ > --- > This is not my code, so it technically needs a SOB from Michael. -Andreas (dilfridge) > --- > sysdeps/unix/sysv/linux/sparc/sparc32/clone.S | 3 +++ > sysdeps/unix/sysv/linux/sparc/sparc64/clone.S | 3 +++ > 2 files changed, 6 insertions(+) > > diff --git a/sysdeps/unix/sysv/linux/sparc/sparc32/clone.S b/sysdeps/unix/sysv/linux/sparc/sparc32/clone.S > index 748d25fcfe..c9cf9bb055 100644 > --- a/sysdeps/unix/sysv/linux/sparc/sparc32/clone.S > +++ b/sysdeps/unix/sysv/linux/sparc/sparc32/clone.S > @@ -28,6 +28,9 @@ > .text > ENTRY (__clone) > save %sp,-96,%sp > + save %sp,-96,%sp > + flushw > + restore > cfi_def_cfa_register(%fp) > cfi_window_save > cfi_register(%o7, %i7) This causes sparcv8-linux-gnu-leon3 fail to build with: ../sysdeps/unix/sysv/linux/sparc/sparc32/clone.S: Assembler messages: ../sysdeps/unix/sysv/linux/sparc/sparc32/clone.S:32: Error: Architecture mismatch on "flushw ". ../sysdeps/unix/sysv/linux/sparc/sparc32/clone.S:32: (Requires v9|v9a|v9b|v9c|v9d|v9e|v9v|v9m|m8; requested architecture is leon.) What should we do about it? Add #ifdef __sparcv9 around the workaround? Thanks, Florian
On 02/08/24 09:10, Florian Weimer wrote: > * Andreas K. Hüttel: > >> From: Michael Karcher <sourceware-bugzilla@mkarcher.dialup.fu-berlin.de> >> >> Bug: https://www.mail-archive.com/debian-glibc@lists.debian.org/msg62592.html >> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31394 >> See-also: https://lore.kernel.org/sparclinux/62f9be9d-a086-4134-9a9f-5df8822708af@mkarcher.dialup.fu-berlin.de/ >> --- >> This is not my code, so it technically needs a SOB from Michael. -Andreas (dilfridge) >> --- >> sysdeps/unix/sysv/linux/sparc/sparc32/clone.S | 3 +++ >> sysdeps/unix/sysv/linux/sparc/sparc64/clone.S | 3 +++ >> 2 files changed, 6 insertions(+) >> >> diff --git a/sysdeps/unix/sysv/linux/sparc/sparc32/clone.S b/sysdeps/unix/sysv/linux/sparc/sparc32/clone.S >> index 748d25fcfe..c9cf9bb055 100644 >> --- a/sysdeps/unix/sysv/linux/sparc/sparc32/clone.S >> +++ b/sysdeps/unix/sysv/linux/sparc/sparc32/clone.S >> @@ -28,6 +28,9 @@ >> .text >> ENTRY (__clone) >> save %sp,-96,%sp >> + save %sp,-96,%sp >> + flushw >> + restore >> cfi_def_cfa_register(%fp) >> cfi_window_save >> cfi_register(%o7, %i7) > > This causes sparcv8-linux-gnu-leon3 fail to build with: > > ../sysdeps/unix/sysv/linux/sparc/sparc32/clone.S: Assembler messages: > ../sysdeps/unix/sysv/linux/sparc/sparc32/clone.S:32: Error: Architecture mismatch on "flushw ". > ../sysdeps/unix/sysv/linux/sparc/sparc32/clone.S:32: (Requires v9|v9a|v9b|v9c|v9d|v9e|v9v|v9m|m8; requested architecture is leon.) > > What should we do about it? Add #ifdef __sparcv9 around the workaround? > Maybe use 'ta 3' for pre sparcv9 which seems what gcc uses for flush_register_window?
Am 02.08.2024 um 15:16 schrieb Adhemerval Zanella Netto: > On 02/08/24 09:10, Florian Weimer wrote: >> * Andreas K. Hüttel: >>> + save %sp,-96,%sp >>> + flushw >>> + restore >> This causes sparcv8-linux-gnu-leon3 fail to build with: >> >> ../sysdeps/unix/sysv/linux/sparc/sparc32/clone.S: Assembler messages: >> ../sysdeps/unix/sysv/linux/sparc/sparc32/clone.S:32: Error: Architecture mismatch on "flushw ". >> ../sysdeps/unix/sysv/linux/sparc/sparc32/clone.S:32: (Requires v9|v9a|v9b|v9c|v9d|v9e|v9v|v9m|m8; requested architecture is leon.) >> >> What should we do about it? Add #ifdef __sparcv9 around the workaround? > Maybe use 'ta 3' for pre sparcv9 which seems what gcc uses for flush_register_window? I suggest to try to reproduce the underlying issue on Leon. You can use the reproducer in tbe bugzilla item as starting point, but you likely have to remove the SPARC64_STACK_BIAS stuff from it, and furthermore adjust the wasteamount calculation, as it currently contains the Sparc64 page size minus 1 in the bit mask 0x1FFF, and the size of a void pointer in the constant 8. Also, the value 1024 that appears in the increment of the for loop and the printf call later is the page size divided by the size of a void pointer, and might also need adjustment for "optimal experience" with that reproducer. If I understand it correctly, you can run Sparc32 programs on Sparc64 systems, and it already is established that the Sparc64 kernel does need the flushw before invoking clone(), even on 32-bit userspace. If we expect the leon-compatible libc to ever be executed on Sparc64 systems, I don't think just diabling the work-around is a proper solution. If using "ta 3" works with 64-bit kernels as well (but it is likely slower than flushw), using ta3 if we can't be sure the binary will run on v9, and flushw if we can seems like the correct strategy forwards. John Paul Adrian, would you be able to test whether "ta 3" instead of "flushw" in sparc32/clone.s works on the Sparc 64 system we discovered the issue on and report back the result? Maybe you/we can also prepare an official 32-bit version of the demo tool. Do we have a physical leon3-based system at hand to test whether the workaround is required at all for leon, and whether "ta 3" works? Kind regards, Michael Karcher
Am 03.08.2024 um 13:14 schrieb Michael.Karcher: > Am 02.08.2024 um 15:16 schrieb Adhemerval Zanella Netto: >> On 02/08/24 09:10, Florian Weimer wrote: >>> * Andreas K. Hüttel: >>>> + save %sp,-96,%sp >>>> + flushw >>>> + restore >>> This causes sparcv8-linux-gnu-leon3 fail to build with: >>> >>> ../sysdeps/unix/sysv/linux/sparc/sparc32/clone.S: Assembler messages: >>> ../sysdeps/unix/sysv/linux/sparc/sparc32/clone.S:32: Error: Architecture mismatch on "flushw ". >>> ../sysdeps/unix/sysv/linux/sparc/sparc32/clone.S:32: (Requires v9|v9a|v9b|v9c|v9d|v9e|v9v|v9m|m8; requested architecture is leon.) >>> >>> What should we do about it? Add #ifdef __sparcv9 around the workaround? >> Maybe use 'ta 3' for pre sparcv9 which seems what gcc uses for flush_register_window? > John Paul Adrian, would you be able to test whether "ta 3" instead of "flushw" in > sparc32/clone.s works on the Sparc 64 system we discovered the issue on and > report back the result? Maybe you/we can also prepare an official 32-bit version > of the demo tool. As commented in bugzilla, we tested the behaviour on a Sparc V9 host, and were able to reproduce the behaviour reliably for both 32-bit and 64-bit binaries if a libc without workaround was used. Either flushw or ta 3 will fix the issue (but a nop in that place instead of the flushw or ta 3 instruction) will not work, proving that it is indeed the flushw/ta 3 that makes clone work reliably. As a result, I recommend to use flushw on v9 only, and fall back to ta 3 on v8 or earlier. Thanks to Adhemerval Zanella Netto for the suggestion to use ta 3. Kind regards, Michael Karcher
* Michael Karcher: > As commented in bugzilla, we tested the behaviour on a Sparc V9 host, > and were able to reproduce the behaviour reliably for both 32-bit and > 64-bit binaries if a libc without workaround was used. Either flushw > or ta 3 will fix the issue (but a nop in that place instead of the > flushw or ta 3 instruction) will not work, proving that it is indeed > the flushw/ta 3 that makes clone work reliably. > > As a result, I recommend to use flushw on v9 only, and fall back to ta > 3 on v8 or earlier. Thanks to Adhemerval Zanella Netto for the > suggestion to use ta 3. What will happen on SPARC v8 LEON CPUs? Will they be able to execute the “ta 3” instruction? Or do we need to add run-time dispatch? Thanks, Florian
On Sat, 2024-08-03 at 17:18 +0200, Florian Weimer wrote: > * Michael Karcher: > > > As commented in bugzilla, we tested the behaviour on a Sparc V9 host, > > and were able to reproduce the behaviour reliably for both 32-bit and > > 64-bit binaries if a libc without workaround was used. Either flushw > > or ta 3 will fix the issue (but a nop in that place instead of the > > flushw or ta 3 instruction) will not work, proving that it is indeed > > the flushw/ta 3 that makes clone work reliably. > > > > As a result, I recommend to use flushw on v9 only, and fall back to ta > > 3 on v8 or earlier. Thanks to Adhemerval Zanella Netto for the > > suggestion to use ta 3. > > What will happen on SPARC v8 LEON CPUs? Will they be able to execute > the “ta 3” instruction? Or do we need to add run-time dispatch? I think that "ta 3" works already on SPARC v7 meaning it should work on all SPARC CPUs, including the SPARC Leon v8 unless I am misunderstanding this. Adrian
Am 03.08.2024 um 17:18 schrieb Florian Weimer: > * Michael Karcher: > >> As commented in bugzilla, we tested the behaviour on a Sparc V9 host, >> and were able to reproduce the behaviour reliably for both 32-bit and >> 64-bit binaries if a libc without workaround was used. Either flushw >> or ta 3 will fix the issue (but a nop in that place instead of the >> flushw or ta 3 instruction) will not work, proving that it is indeed >> the flushw/ta 3 that makes clone work reliably. >> >> As a result, I recommend to use flushw on v9 only, and fall back to ta >> 3 on v8 or earlier. Thanks to Adhemerval Zanella Netto for the >> suggestion to use ta 3. > What will happen on SPARC v8 LEON CPUs? Will they be able to execute > the “ta 3” instruction? Or do we need to add run-time dispatch? > > Thanks, > Florian "ta 3" invokes the kernel handler for CPU trap number 3. The Sparc Processor Supplement to the System V ABI defines: "By executing a type 3 trap, a process asks the system to flush all its register windows to the stack". The "ta" instruction is implemented on all remotely relevant SPARC CPUs, and there is no reason to believe that Linux on LEON chose to not implement this aspect of the System V ABI. With SPARC v9, the new machine instruction FLUSHW causes a "spill trap" if there are any dirty windows below the current one. A spill trap should be handled the same way as the user-defined trap type 3 by the kernel. In the clone issue workaround, the extra save/restore pair causes the window of the userspace clone function to already be below the current window, and as that window is also freshly opened, it is surely not yet flushed so flushw will always raise a spill trap. Looking at it a second time, it is likely that we don't need the extra save/restore pair around the "ta 3" instruction, which I just confirmed by replacing the extra save/restore pair with NOPs. I will leave it to the SPARC v9 experts to judge whether "save/flushw/restore" or "ta 3" is the "better" approach on v9. On v8, "ta 3" is the only possible approach. So these additions to the prologue of clone have been positively tested: - save/flushw/restore (requires SPARC v9) - save/ta 3/restore - ta 3 And these additions have been negatively tested: - flushw only (requires SPARC v9) - no addition at all Kind regards, Michael Karcher
diff --git a/sysdeps/unix/sysv/linux/sparc/sparc32/clone.S b/sysdeps/unix/sysv/linux/sparc/sparc32/clone.S index 748d25fcfe..c9cf9bb055 100644 --- a/sysdeps/unix/sysv/linux/sparc/sparc32/clone.S +++ b/sysdeps/unix/sysv/linux/sparc/sparc32/clone.S @@ -28,6 +28,9 @@ .text ENTRY (__clone) save %sp,-96,%sp + save %sp,-96,%sp + flushw + restore cfi_def_cfa_register(%fp) cfi_window_save cfi_register(%o7, %i7) diff --git a/sysdeps/unix/sysv/linux/sparc/sparc64/clone.S b/sysdeps/unix/sysv/linux/sparc/sparc64/clone.S index e5ff2cf1a0..370d51fda2 100644 --- a/sysdeps/unix/sysv/linux/sparc/sparc64/clone.S +++ b/sysdeps/unix/sysv/linux/sparc/sparc64/clone.S @@ -32,6 +32,9 @@ ENTRY (__clone) save %sp, -192, %sp + save %sp, -192, %sp + flushw + restore cfi_def_cfa_register(%fp) cfi_window_save cfi_register(%o7, %i7)
From: Michael Karcher <sourceware-bugzilla@mkarcher.dialup.fu-berlin.de> Bug: https://www.mail-archive.com/debian-glibc@lists.debian.org/msg62592.html Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31394 See-also: https://lore.kernel.org/sparclinux/62f9be9d-a086-4134-9a9f-5df8822708af@mkarcher.dialup.fu-berlin.de/ --- This is not my code, so it technically needs a SOB from Michael. -Andreas (dilfridge) --- sysdeps/unix/sysv/linux/sparc/sparc32/clone.S | 3 +++ sysdeps/unix/sysv/linux/sparc/sparc64/clone.S | 3 +++ 2 files changed, 6 insertions(+)