Possible regression by ab037dd87a2f (powerpc/vdso: Switch VDSO to generic C implementation.)

8 messages, 3 authors, 2021-07-29 · open the first message on its own page

Possible regression by ab037dd87a2f (powerpc/vdso: Switch VDSO to generic C implementation.)

From: Paul Menzel <hidden>
Date: 2021-07-27 08:54:50

Dear Christophe,


On ppc64le Go 1.16.2 from Ubuntu 21.04 terminates with a segmentation 
fault [1], and it might be related to *[release-branch.go1.16] runtime: 
fix crash during VDSO calls on PowerPC* [2], conjecturing that commit 
ab037dd87a2f (powerpc/vdso: Switch VDSO to generic C implementation.) 
added in Linux 5.11 causes this.

If this is indeed the case, this would be a regression in userspace. Is 
there a generic fix or should the change be reverted?


Kind regards,

Paul


[1]: https://github.com/9elements/converged-security-suite/issues/268
[2]: https://go-review.googlesource.com/c/go/+/334410/
[3]: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ab037dd87a2f946556850e204c06cbd7a2a19390

Re: Possible regression by ab037dd87a2f (powerpc/vdso: Switch VDSO to generic C implementation.)

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: 2021-07-27 23:18:43

On Tue, 2021-07-27 at 10:45 +0200, Paul Menzel wrote:
Dear Christophe,


On ppc64le Go 1.16.2 from Ubuntu 21.04 terminates with a segmentation 
fault [1], and it might be related to *[release-branch.go1.16] runtime: 
fix crash during VDSO calls on PowerPC* [2], conjecturing that commit 
ab037dd87a2f (powerpc/vdso: Switch VDSO to generic C implementation.) 
added in Linux 5.11 causes this.

If this is indeed the case, this would be a regression in userspace. Is 
there a generic fix or should the change be reverted?
From the look at the links you posted, this appears to be completely
broken assumptions by Go that some registers don't change while calling
what essentially are external library functions *while inside those
functions* (ie in this case from a signal handler).

I suppose it would be possible to build the VDSO with gcc arguments to
make it not use r30, but that's just gross...

Cheers,
Ben.


Re: Possible regression by ab037dd87a2f (powerpc/vdso: Switch VDSO to generic C implementation.)

From: Paul Menzel <hidden>
Date: 2021-07-28 08:27:02

Dear Benjamin,


Am 28.07.21 um 01:14 schrieb Benjamin Herrenschmidt:
On Tue, 2021-07-27 at 10:45 +0200, Paul Menzel wrote:
quoted
On ppc64le Go 1.16.2 from Ubuntu 21.04 terminates with a segmentation
fault [1], and it might be related to *[release-branch.go1.16] runtime:
fix crash during VDSO calls on PowerPC* [2], conjecturing that commit
ab037dd87a2f (powerpc/vdso: Switch VDSO to generic C implementation.)
added in Linux 5.11 causes this.

If this is indeed the case, this would be a regression in userspace. Is
there a generic fix or should the change be reverted?
From the look at the links you posted, this appears to be completely
broken assumptions by Go that some registers don't change while calling
what essentially are external library functions *while inside those
functions* (ie in this case from a signal handler).

I suppose it would be possible to build the VDSO with gcc arguments to
make it not use r30, but that's just gross...
Thank you for looking into this. No idea, if it falls under Linux’ no 
regression policy or not.


Kind regards,

Paul

Re: Possible regression by ab037dd87a2f (powerpc/vdso: Switch VDSO to generic C implementation.)

From: Michael Ellerman <mpe@ellerman.id.au>
Date: 2021-07-28 12:44:11

Paul Menzel [off-list ref] writes:
Am 28.07.21 um 01:14 schrieb Benjamin Herrenschmidt:
quoted
On Tue, 2021-07-27 at 10:45 +0200, Paul Menzel wrote:
quoted
quoted
On ppc64le Go 1.16.2 from Ubuntu 21.04 terminates with a segmentation
fault [1], and it might be related to *[release-branch.go1.16] runtime:
fix crash during VDSO calls on PowerPC* [2], conjecturing that commit
ab037dd87a2f (powerpc/vdso: Switch VDSO to generic C implementation.)
added in Linux 5.11 causes this.

If this is indeed the case, this would be a regression in userspace. Is
there a generic fix or should the change be reverted?
From the look at the links you posted, this appears to be completely
broken assumptions by Go that some registers don't change while calling
what essentially are external library functions *while inside those
functions* (ie in this case from a signal handler).

I suppose it would be possible to build the VDSO with gcc arguments to
make it not use r30, but that's just gross...
Thank you for looking into this. No idea, if it falls under Linux’ no 
regression policy or not.
Reluctantly yes, I think it does. Though it would have been good if it
had been reported to us sooner.

It looks like that Go fix is only committed to master, and neither of
the latest Go 1.16 or 1.15 releases contain the fix? ie. there's no way
for a user to get a working version of Go other than building master?

I'll see if we can work around it in the kernel. Are you able to test a
kernel patch if I send you one?

cheers

Re: Possible regression by ab037dd87a2f (powerpc/vdso: Switch VDSO to generic C implementation.)

From: Paul Menzel <hidden>
Date: 2021-07-28 12:53:33

Dear Michael,


Am 28.07.21 um 14:43 schrieb Michael Ellerman:
Paul Menzel [off-list ref] writes:
quoted
Am 28.07.21 um 01:14 schrieb Benjamin Herrenschmidt:
quoted
On Tue, 2021-07-27 at 10:45 +0200, Paul Menzel wrote:
quoted
quoted
On ppc64le Go 1.16.2 from Ubuntu 21.04 terminates with a segmentation
fault [1], and it might be related to *[release-branch.go1.16] runtime:
fix crash during VDSO calls on PowerPC* [2], conjecturing that commit
ab037dd87a2f (powerpc/vdso: Switch VDSO to generic C implementation.)
added in Linux 5.11 causes this.

If this is indeed the case, this would be a regression in userspace. Is
there a generic fix or should the change be reverted?
 From the look at the links you posted, this appears to be completely
broken assumptions by Go that some registers don't change while calling
what essentially are external library functions *while inside those
functions* (ie in this case from a signal handler).

I suppose it would be possible to build the VDSO with gcc arguments to
make it not use r30, but that's just gross...
Thank you for looking into this. No idea, if it falls under Linux’ no
regression policy or not.
Reluctantly yes, I think it does. Though it would have been good if it
had been reported to us sooner.

It looks like that Go fix is only committed to master, and neither of
the latest Go 1.16 or 1.15 releases contain the fix? ie. there's no way
for a user to get a working version of Go other than building master?
I heard it is going to be in Go 1.16.7, but I do not know much about Go. 
Maybe the folks in Cc can chime in.
I'll see if we can work around it in the kernel. Are you able to test a
kernel patch if I send you one?
Yes, I could test a Linux kernel patch on ppc64le (POWER 8) running 
Ubuntu 21.04.


Kind regards,

Paul

Re: Possible regression by ab037dd87a2f (powerpc/vdso: Switch VDSO to generic C implementation.)

From: Michael Ellerman <mpe@ellerman.id.au>
Date: 2021-07-29 07:41:56

Paul Menzel [off-list ref] writes:
Dear Michael,


Am 28.07.21 um 14:43 schrieb Michael Ellerman:
quoted
Paul Menzel [off-list ref] writes:
quoted
Am 28.07.21 um 01:14 schrieb Benjamin Herrenschmidt:
quoted
On Tue, 2021-07-27 at 10:45 +0200, Paul Menzel wrote:
quoted
quoted
On ppc64le Go 1.16.2 from Ubuntu 21.04 terminates with a segmentation
fault [1], and it might be related to *[release-branch.go1.16] runtime:
fix crash during VDSO calls on PowerPC* [2], conjecturing that commit
ab037dd87a2f (powerpc/vdso: Switch VDSO to generic C implementation.)
added in Linux 5.11 causes this.

If this is indeed the case, this would be a regression in userspace. Is
there a generic fix or should the change be reverted?
 From the look at the links you posted, this appears to be completely
broken assumptions by Go that some registers don't change while calling
what essentially are external library functions *while inside those
functions* (ie in this case from a signal handler).

I suppose it would be possible to build the VDSO with gcc arguments to
make it not use r30, but that's just gross...
Thank you for looking into this. No idea, if it falls under Linux’ no
regression policy or not.
Reluctantly yes, I think it does. Though it would have been good if it
had been reported to us sooner.

It looks like that Go fix is only committed to master, and neither of
the latest Go 1.16 or 1.15 releases contain the fix? ie. there's no way
for a user to get a working version of Go other than building master?
I heard it is going to be in Go 1.16.7, but I do not know much about Go. 
Maybe the folks in Cc can chime in.
quoted
I'll see if we can work around it in the kernel. Are you able to test a
kernel patch if I send you one?
Yes, I could test a Linux kernel patch on ppc64le (POWER 8) running 
Ubuntu 21.04.
Thanks, would be great if you can test on your setup. Patch below.

I haven't been able to reproduce the crash by following the instructions
in your bug report, I have go1.13.8, I guess the crash is only in newer
versions?

cheers

diff --git a/arch/powerpc/kernel/vdso64/Makefile b/arch/powerpc/kernel/vdso64/Makefile
index 2813e3f98db6..3c5baaa6f1e7 100644
--- a/arch/powerpc/kernel/vdso64/Makefile
+++ b/arch/powerpc/kernel/vdso64/Makefile
@@ -27,6 +27,13 @@ KASAN_SANITIZE := n
 
 ccflags-y := -shared -fno-common -fno-builtin -nostdlib \
 	-Wl,-soname=linux-vdso64.so.1 -Wl,--hash-style=both
+
+# Go prior to 1.16.x assumes r30 is not clobbered by any VDSO code. That used to be true
+# by accident when the VDSO was hand-written asm code, but may not be now that the VDSO is
+# compiler generated. To avoid breaking Go tell GCC not to use r30. Impact on code
+# generation is minimal, it will just use r29 instead.
+ccflags-y += $(call cc-option, -ffixed-r30)
+
 asflags-y := -D__VDSO64__ -s
 
 targets += vdso64.lds

Re: Possible regression by ab037dd87a2f (powerpc/vdso: Switch VDSO to generic C implementation.)

From: Paul Menzel <hidden>
Date: 2021-07-29 08:33:43

Dear Michael,


Am 29.07.21 um 09:41 schrieb Michael Ellerman:
Paul Menzel [off-list ref] writes:
quoted
Am 28.07.21 um 14:43 schrieb Michael Ellerman:
quoted
Paul Menzel [off-list ref] writes:
quoted
Am 28.07.21 um 01:14 schrieb Benjamin Herrenschmidt:
quoted
On Tue, 2021-07-27 at 10:45 +0200, Paul Menzel wrote:
quoted
quoted
On ppc64le Go 1.16.2 from Ubuntu 21.04 terminates with a segmentation
fault [1], and it might be related to *[release-branch.go1.16] runtime:
fix crash during VDSO calls on PowerPC* [2], conjecturing that commit
ab037dd87a2f (powerpc/vdso: Switch VDSO to generic C implementation.)
added in Linux 5.11 causes this.

If this is indeed the case, this would be a regression in userspace. Is
there a generic fix or should the change be reverted?
  From the look at the links you posted, this appears to be completely
broken assumptions by Go that some registers don't change while calling
what essentially are external library functions *while inside those
functions* (ie in this case from a signal handler).

I suppose it would be possible to build the VDSO with gcc arguments to
make it not use r30, but that's just gross...
Thank you for looking into this. No idea, if it falls under Linux’ no
regression policy or not.
Reluctantly yes, I think it does. Though it would have been good if it
had been reported to us sooner.

It looks like that Go fix is only committed to master, and neither of
the latest Go 1.16 or 1.15 releases contain the fix? ie. there's no way
for a user to get a working version of Go other than building master?
I heard it is going to be in Go 1.16.7, but I do not know much about Go.
Maybe the folks in Cc can chime in.
quoted
I'll see if we can work around it in the kernel. Are you able to test a
kernel patch if I send you one?
Yes, I could test a Linux kernel patch on ppc64le (POWER 8) running
Ubuntu 21.04.
Thanks, would be great if you can test on your setup. Patch below.

I haven't been able to reproduce the crash by following the instructions
in your bug report, I have go1.13.8, I guess the crash is only in newer
versions?
I only used go version 1.16.2 packaged in Ubuntu 21.04 (1.16~0ubuntu1).
quoted hunk
diff --git a/arch/powerpc/kernel/vdso64/Makefile b/arch/powerpc/kernel/vdso64/Makefile
index 2813e3f98db6..3c5baaa6f1e7 100644
--- a/arch/powerpc/kernel/vdso64/Makefile
+++ b/arch/powerpc/kernel/vdso64/Makefile
@@ -27,6 +27,13 @@ KASAN_SANITIZE := n
  
  ccflags-y := -shared -fno-common -fno-builtin -nostdlib \
  	-Wl,-soname=linux-vdso64.so.1 -Wl,--hash-style=both
+
+# Go prior to 1.16.x assumes r30 is not clobbered by any VDSO code. That used to be true
Probably that needs to be 1.16.7.
+# by accident when the VDSO was hand-written asm code, but may not be now that the VDSO is
+# compiler generated. To avoid breaking Go tell GCC not to use r30. Impact on code
+# generation is minimal, it will just use r29 instead.
+ccflags-y += $(call cc-option, -ffixed-r30)
+
  asflags-y := -D__VDSO64__ -s
  
  targets += vdso64.lds
With this applied to Linux, go does not crash with a segmentation fault 
anymore.

Tested-by: Paul Menzel <redacted>

(Probably the commit should be tagged for the stable series too.)


Kind regards,

Paul

Re: Possible regression by ab037dd87a2f (powerpc/vdso: Switch VDSO to generic C implementation.)

From: Michael Ellerman <mpe@ellerman.id.au>
Date: 2021-07-29 12:59:36

Paul Menzel [off-list ref] writes:
Am 29.07.21 um 09:41 schrieb Michael Ellerman:
quoted
Paul Menzel [off-list ref] writes:
quoted
quoted
Am 28.07.21 um 14:43 schrieb Michael Ellerman:
quoted
Paul Menzel [off-list ref] writes:
quoted
Am 28.07.21 um 01:14 schrieb Benjamin Herrenschmidt:
quoted
On Tue, 2021-07-27 at 10:45 +0200, Paul Menzel wrote:
quoted
quoted
On ppc64le Go 1.16.2 from Ubuntu 21.04 terminates with a segmentation
fault [1], and it might be related to *[release-branch.go1.16] runtime:
fix crash during VDSO calls on PowerPC* [2], conjecturing that commit
ab037dd87a2f (powerpc/vdso: Switch VDSO to generic C implementation.)
added in Linux 5.11 causes this.

If this is indeed the case, this would be a regression in userspace. Is
there a generic fix or should the change be reverted?
  From the look at the links you posted, this appears to be completely
broken assumptions by Go that some registers don't change while calling
what essentially are external library functions *while inside those
functions* (ie in this case from a signal handler).

I suppose it would be possible to build the VDSO with gcc arguments to
make it not use r30, but that's just gross...
Thank you for looking into this. No idea, if it falls under Linux’ no
regression policy or not.
Reluctantly yes, I think it does. Though it would have been good if it
had been reported to us sooner.

It looks like that Go fix is only committed to master, and neither of
the latest Go 1.16 or 1.15 releases contain the fix? ie. there's no way
for a user to get a working version of Go other than building master?
I heard it is going to be in Go 1.16.7, but I do not know much about Go.
Maybe the folks in Cc can chime in.
quoted
I'll see if we can work around it in the kernel. Are you able to test a
kernel patch if I send you one?
Yes, I could test a Linux kernel patch on ppc64le (POWER 8) running
Ubuntu 21.04.
Thanks, would be great if you can test on your setup. Patch below.

I haven't been able to reproduce the crash by following the instructions
in your bug report, I have go1.13.8, I guess the crash is only in newer
versions?
I only used go version 1.16.2 packaged in Ubuntu 21.04 (1.16~0ubuntu1).
OK thanks. I can reproduce with that.
quoted
diff --git a/arch/powerpc/kernel/vdso64/Makefile b/arch/powerpc/kernel/vdso64/Makefile
index 2813e3f98db6..3c5baaa6f1e7 100644
--- a/arch/powerpc/kernel/vdso64/Makefile
+++ b/arch/powerpc/kernel/vdso64/Makefile
@@ -27,6 +27,13 @@ KASAN_SANITIZE := n
  
  ccflags-y := -shared -fno-common -fno-builtin -nostdlib \
  	-Wl,-soname=linux-vdso64.so.1 -Wl,--hash-style=both
+
+# Go prior to 1.16.x assumes r30 is not clobbered by any VDSO code. That used to be true
Probably that needs to be 1.16.7.
I made it 1.16.x because it wasn't 100% clear on whether the fix will
land in 1.16.7 or not.

For our purposes 1.16.x is good enough, we'll need to carry the
workaround until 1.16 is well and truly EOL, so it doesn't really matter
which point release it's in.
quoted
+# by accident when the VDSO was hand-written asm code, but may not be now that the VDSO is
+# compiler generated. To avoid breaking Go tell GCC not to use r30. Impact on code
+# generation is minimal, it will just use r29 instead.
+ccflags-y += $(call cc-option, -ffixed-r30)
+
  asflags-y := -D__VDSO64__ -s
  
  targets += vdso64.lds
With this applied to Linux, go does not crash with a segmentation fault 
anymore.

Tested-by: Paul Menzel <redacted>
Thanks.
(Probably the commit should be tagged for the stable series too.)
Yep.

cheers
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help