All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] powerpc/vdso: Don't use r30 to avoid breaking Go lang
@ 2021-07-29 13:12 Michael Ellerman
  2021-07-29 13:42 ` Paul Menzel
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Michael Ellerman @ 2021-07-29 13:12 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: pmenzel

The Go runtime uses r30 for some special value called 'g'. It assumes
that value will remain unchanged even when calling VDSO functions.
Although r30 is non-volatile across function calls, the callee is free
to use it, as long as the callee saves the value and restores it before
returning.

It used to be true by accident that the VDSO didn't use r30, because the
VDSO was hand-written asm. When we switched to building the VDSO from C
the compiler started using r30, at least in some builds, leading to
crashes in Go. eg:

  ~/go/src$ ./all.bash
  Building Go cmd/dist using /usr/lib/go-1.16. (go1.16.2 linux/ppc64le)
  Building Go toolchain1 using /usr/lib/go-1.16.
  go build os/exec: /usr/lib/go-1.16/pkg/tool/linux_ppc64le/compile: signal: segmentation fault
  go build reflect: /usr/lib/go-1.16/pkg/tool/linux_ppc64le/compile: signal: segmentation fault
  go tool dist: FAILED: /usr/lib/go-1.16/bin/go install -gcflags=-l -tags=math_big_pure_go compiler_bootstrap bootstrap/cmd/...: exit status 1

There are patches in flight to fix Go[1], but until they are released
and widely deployed we can workaround it in the VDSO by avoiding use of
r30.

Note this only works with GCC, clang does not support -ffixed-rN.

1: https://go-review.googlesource.com/c/go/+/328110

Fixes: ab037dd87a2f ("powerpc/vdso: Switch VDSO to generic C implementation.")
Cc: stable@vger.kernel.org # v5.11+
Reported-by: Paul Menzel <pmenzel@molgen.mpg.de>
Tested-by: Paul Menzel <pmenzel@molgen.mpg.de>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/kernel/vdso64/Makefile | 7 +++++++
 1 file changed, 7 insertions(+)

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
-- 
2.25.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] powerpc/vdso: Don't use r30 to avoid breaking Go lang
  2021-07-29 13:12 [PATCH] powerpc/vdso: Don't use r30 to avoid breaking Go lang Michael Ellerman
@ 2021-07-29 13:42 ` Paul Menzel
  2021-07-29 16:25   ` Nick Desaulniers
  2021-08-01 13:14 ` Michael Ellerman
  2021-08-02 11:27 ` Michael Ellerman
  2 siblings, 1 reply; 6+ messages in thread
From: Paul Menzel @ 2021-07-29 13:42 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: Nathan Chancellor, Nick Desaulniers, linuxppc-dev, clang-built-linux

Dear Michael,


Am 29.07.21 um 15:12 schrieb Michael Ellerman:
> The Go runtime uses r30 for some special value called 'g'. It assumes
> that value will remain unchanged even when calling VDSO functions.
> Although r30 is non-volatile across function calls, the callee is free
> to use it, as long as the callee saves the value and restores it before
> returning.
> 
> It used to be true by accident that the VDSO didn't use r30, because the
> VDSO was hand-written asm. When we switched to building the VDSO from C
> the compiler started using r30, at least in some builds, leading to
> crashes in Go. eg:
> 
>    ~/go/src$ ./all.bash
>    Building Go cmd/dist using /usr/lib/go-1.16. (go1.16.2 linux/ppc64le)
>    Building Go toolchain1 using /usr/lib/go-1.16.
>    go build os/exec: /usr/lib/go-1.16/pkg/tool/linux_ppc64le/compile: signal: segmentation fault
>    go build reflect: /usr/lib/go-1.16/pkg/tool/linux_ppc64le/compile: signal: segmentation fault
>    go tool dist: FAILED: /usr/lib/go-1.16/bin/go install -gcflags=-l -tags=math_big_pure_go compiler_bootstrap bootstrap/cmd/...: exit status 1
> 
> There are patches in flight to fix Go[1], but until they are released
> and widely deployed we can workaround it in the VDSO by avoiding use of

Nit: work around is spelled with a space.

> r30.
> 
> Note this only works with GCC, clang does not support -ffixed-rN.

Maybe the clang/LLVM build support folks (in CC) have an idea.

> 1: https://go-review.googlesource.com/c/go/+/328110
> 
> Fixes: ab037dd87a2f ("powerpc/vdso: Switch VDSO to generic C implementation.")
> Cc: stable@vger.kernel.org # v5.11+
> Reported-by: Paul Menzel <pmenzel@molgen.mpg.de>
> Tested-by: Paul Menzel <pmenzel@molgen.mpg.de>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>   arch/powerpc/kernel/vdso64/Makefile | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> 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
> 

The rest looks good.


Kind regards,

Paul

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] powerpc/vdso: Don't use r30 to avoid breaking Go lang
  2021-07-29 13:42 ` Paul Menzel
@ 2021-07-29 16:25   ` Nick Desaulniers
  2021-07-30 23:55     ` Segher Boessenkool
  0 siblings, 1 reply; 6+ messages in thread
From: Nick Desaulniers @ 2021-07-29 16:25 UTC (permalink / raw)
  To: Paul Menzel; +Cc: Nathan Chancellor, linuxppc-dev, clang-built-linux

On Thu, Jul 29, 2021 at 6:42 AM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
>
> Dear Michael,
>
>
> Am 29.07.21 um 15:12 schrieb Michael Ellerman:
> > The Go runtime uses r30 for some special value called 'g'. It assumes
> > that value will remain unchanged even when calling VDSO functions.
> > Although r30 is non-volatile across function calls, the callee is free
> > to use it, as long as the callee saves the value and restores it before
> > returning.
> >
> > It used to be true by accident that the VDSO didn't use r30, because the
> > VDSO was hand-written asm. When we switched to building the VDSO from C
> > the compiler started using r30, at least in some builds, leading to
> > crashes in Go. eg:
> >
> >    ~/go/src$ ./all.bash
> >    Building Go cmd/dist using /usr/lib/go-1.16. (go1.16.2 linux/ppc64le)
> >    Building Go toolchain1 using /usr/lib/go-1.16.
> >    go build os/exec: /usr/lib/go-1.16/pkg/tool/linux_ppc64le/compile: signal: segmentation fault
> >    go build reflect: /usr/lib/go-1.16/pkg/tool/linux_ppc64le/compile: signal: segmentation fault
> >    go tool dist: FAILED: /usr/lib/go-1.16/bin/go install -gcflags=-l -tags=math_big_pure_go compiler_bootstrap bootstrap/cmd/...: exit status 1
> >
> > There are patches in flight to fix Go[1], but until they are released
> > and widely deployed we can workaround it in the VDSO by avoiding use of
>
> Nit: work around is spelled with a space.
>
> > r30.
> >
> > Note this only works with GCC, clang does not support -ffixed-rN.
>
> Maybe the clang/LLVM build support folks (in CC) have an idea.

Right, we've had issues with these in the past.  Generally, we need to
teach clang about which registers are valid for `N` so that it can
diagnose invalid values ASAP.  This has to be done on a per arch basis
in LLVM to steal the register from the register allocator.  For
example, this was used previously for aarch64 (but removed from use in
the kernel) and IIRC is used for m68k (which we're working to get
builds online for).

I've filed https://bugs.llvm.org/show_bug.cgi?id=51272. Thanks for the report.

>
> > 1: https://go-review.googlesource.com/c/go/+/328110
> >
> > Fixes: ab037dd87a2f ("powerpc/vdso: Switch VDSO to generic C implementation.")
> > Cc: stable@vger.kernel.org # v5.11+
> > Reported-by: Paul Menzel <pmenzel@molgen.mpg.de>
> > Tested-by: Paul Menzel <pmenzel@molgen.mpg.de>
> > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> > ---
> >   arch/powerpc/kernel/vdso64/Makefile | 7 +++++++
> >   1 file changed, 7 insertions(+)
> >
> > 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
> >
>
> The rest looks good.
>
>
> Kind regards,
>
> Paul



-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] powerpc/vdso: Don't use r30 to avoid breaking Go lang
  2021-07-29 16:25   ` Nick Desaulniers
@ 2021-07-30 23:55     ` Segher Boessenkool
  0 siblings, 0 replies; 6+ messages in thread
From: Segher Boessenkool @ 2021-07-30 23:55 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Nathan Chancellor, Paul Menzel, linuxppc-dev, clang-built-linux

On Thu, Jul 29, 2021 at 09:25:43AM -0700, Nick Desaulniers wrote:
> On Thu, Jul 29, 2021 at 6:42 AM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
> > Am 29.07.21 um 15:12 schrieb Michael Ellerman:
> > > Note this only works with GCC, clang does not support -ffixed-rN.
> >
> > Maybe the clang/LLVM build support folks (in CC) have an idea.
> 
> Right, we've had issues with these in the past.  Generally, we need to
> teach clang about which registers are valid for `N` so that it can
> diagnose invalid values ASAP.  This has to be done on a per arch basis
> in LLVM to steal the register from the register allocator.  For
> example, this was used previously for aarch64 (but removed from use in
> the kernel) and IIRC is used for m68k (which we're working to get
> builds online for).

In GCC, it is -ffixed-* (note: no "r").  The string is stripped of the
standard prefix for the target (for Power, none), and possibly of one
"%" or "#".  If the string is a recognised register name (or alternative
register name) for the target, that is used.  If not, and it is a
decimal number, then the internal GCC register of that number is used
(these numbers can differ from one GCC release to another, and in fact
we have changed the numbering for Power before -- but 0..31 have always
been the GPRs, and 32..63 have always been the FPRs).

The names for the Power registers are:
GPRs:
  0..31, or alternatively
  r0..r31
FPRs:
  0..31, or alternatively
  fr0..fr31, or alternatively
  vs0..vs31
VRs:
  0..31, or alternatively
  v0..v31, or alternatively
  vs32..vs63
CR fields:
  0..7, or alternatively
  cr0..cr7

(There are more, but changing the default calling convention for those
will not work anyway.  Also, some non-Linux configurations use different
names.)


Segher

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] powerpc/vdso: Don't use r30 to avoid breaking Go lang
  2021-07-29 13:12 [PATCH] powerpc/vdso: Don't use r30 to avoid breaking Go lang Michael Ellerman
  2021-07-29 13:42 ` Paul Menzel
@ 2021-08-01 13:14 ` Michael Ellerman
  2021-08-02 11:27 ` Michael Ellerman
  2 siblings, 0 replies; 6+ messages in thread
From: Michael Ellerman @ 2021-08-01 13:14 UTC (permalink / raw)
  To: Michael Ellerman, linuxppc-dev; +Cc: pmenzel

On Thu, 29 Jul 2021 23:12:44 +1000, Michael Ellerman wrote:
> The Go runtime uses r30 for some special value called 'g'. It assumes
> that value will remain unchanged even when calling VDSO functions.
> Although r30 is non-volatile across function calls, the callee is free
> to use it, as long as the callee saves the value and restores it before
> returning.
> 
> It used to be true by accident that the VDSO didn't use r30, because the
> VDSO was hand-written asm. When we switched to building the VDSO from C
> the compiler started using r30, at least in some builds, leading to
> crashes in Go. eg:
> 
> [...]

Applied to powerpc/fixes.

[1/1] powerpc/vdso: Don't use r30 to avoid breaking Go lang
      https://git.kernel.org/powerpc/c/a88603f4b92ecef9e2359e40bcb99ad399d85dd7

cheers

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] powerpc/vdso: Don't use r30 to avoid breaking Go lang
  2021-07-29 13:12 [PATCH] powerpc/vdso: Don't use r30 to avoid breaking Go lang Michael Ellerman
  2021-07-29 13:42 ` Paul Menzel
  2021-08-01 13:14 ` Michael Ellerman
@ 2021-08-02 11:27 ` Michael Ellerman
  2 siblings, 0 replies; 6+ messages in thread
From: Michael Ellerman @ 2021-08-02 11:27 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: pmenzel

Michael Ellerman <mpe@ellerman.id.au> writes:
> The Go runtime uses r30 for some special value called 'g'. It assumes
> that value will remain unchanged even when calling VDSO functions.
> Although r30 is non-volatile across function calls, the callee is free
> to use it, as long as the callee saves the value and restores it before
> returning.
>
> It used to be true by accident that the VDSO didn't use r30, because the
> VDSO was hand-written asm. When we switched to building the VDSO from C
> the compiler started using r30, at least in some builds, leading to
> crashes in Go. eg:
>
>   ~/go/src$ ./all.bash
>   Building Go cmd/dist using /usr/lib/go-1.16. (go1.16.2 linux/ppc64le)
>   Building Go toolchain1 using /usr/lib/go-1.16.
>   go build os/exec: /usr/lib/go-1.16/pkg/tool/linux_ppc64le/compile: signal: segmentation fault
>   go build reflect: /usr/lib/go-1.16/pkg/tool/linux_ppc64le/compile: signal: segmentation fault
>   go tool dist: FAILED: /usr/lib/go-1.16/bin/go install -gcflags=-l -tags=math_big_pure_go compiler_bootstrap bootstrap/cmd/...: exit status 1
>
> There are patches in flight to fix Go[1], but until they are released
> and widely deployed we can workaround it in the VDSO by avoiding use of
> r30.
>
> Note this only works with GCC, clang does not support -ffixed-rN.
>
> 1: https://go-review.googlesource.com/c/go/+/328110
>
> Fixes: ab037dd87a2f ("powerpc/vdso: Switch VDSO to generic C implementation.")
> Cc: stable@vger.kernel.org # v5.11+

In practice, with GCC 10.3.0, that commit doesn't result in r30 being
used by the compiler.

It's commit 74205b3fc2ef ("powerpc/vdso: Add support for time
namespaces"), which went into v5.13-rc1, which causes r30 to be used in
__c_kernel_clock_gettime():

00000000000006e0 <__c_kernel_clock_gettime>:
 6e0:   0f 00 03 28     cmplwi  r3,15
 6e4:   ec 00 81 41     bgt     7d0 <__c_kernel_clock_gettime+0xf0>
 6e8:   01 00 20 39     li      r9,1
 6ec:   30 18 29 7d     slw     r9,r9,r3
 6f0:   83 08 2a 71     andi.   r10,r9,2179
 6f4:   fc 00 82 41     beq     7f0 <__c_kernel_clock_gettime+0x110>
 6f8:   e4 26 63 78     rldicr  r3,r3,4,59
 6fc:   ff 7f 20 3d     lis     r9,32767
 700:   f0 ff c1 fb     std     r30,-16(r1)
 704:   f8 ff e1 fb     std     r31,-8(r1)
 708:   14 1a c5 7c     add     r6,r5,r3
 70c:   ff ff 2b 61     ori     r11,r9,65535
 710:   00 00 05 81     lwz     r8,0(r5)
 714:   01 00 09 71     andi.   r9,r8,1
 718:   98 00 82 40     bne     7b0 <__c_kernel_clock_gettime+0xd0>
 71c:   ac 04 20 7c     lwsync
 720:   a6 42 cc 7f     mftb    r30


cheers

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-08-02 11:28 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-29 13:12 [PATCH] powerpc/vdso: Don't use r30 to avoid breaking Go lang Michael Ellerman
2021-07-29 13:42 ` Paul Menzel
2021-07-29 16:25   ` Nick Desaulniers
2021-07-30 23:55     ` Segher Boessenkool
2021-08-01 13:14 ` Michael Ellerman
2021-08-02 11:27 ` Michael Ellerman

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.