All of lore.kernel.org
 help / color / mirror / Atom feed
* Possible regression by ab037dd87a2f (powerpc/vdso: Switch VDSO to generic C implementation.)
@ 2021-07-27  8:45 Paul Menzel
  2021-07-27 23:14 ` Benjamin Herrenschmidt
  0 siblings, 1 reply; 14+ messages in thread
From: Paul Menzel @ 2021-07-27  8:45 UTC (permalink / raw)
  To: Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras,
	Christophe Leroy
  Cc: Derek Parker, Dmitrii Okunev, linuxppc-dev

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

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

* Re: Possible regression by ab037dd87a2f (powerpc/vdso: Switch VDSO to generic C implementation.)
  2021-07-27  8:45 Possible regression by ab037dd87a2f (powerpc/vdso: Switch VDSO to generic C implementation.) Paul Menzel
@ 2021-07-27 23:14 ` Benjamin Herrenschmidt
  2021-07-28  8:26   ` Paul Menzel
  0 siblings, 1 reply; 14+ messages in thread
From: Benjamin Herrenschmidt @ 2021-07-27 23:14 UTC (permalink / raw)
  To: Paul Menzel, Michael Ellerman, Paul Mackerras, Christophe Leroy
  Cc: Derek Parker, Dmitrii Okunev, linuxppc-dev

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.




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

* Re: Possible regression by ab037dd87a2f (powerpc/vdso: Switch VDSO to generic C implementation.)
  2021-07-27 23:14 ` Benjamin Herrenschmidt
@ 2021-07-28  8:26   ` Paul Menzel
  2021-07-28 12:43     ` Michael Ellerman
  0 siblings, 1 reply; 14+ messages in thread
From: Paul Menzel @ 2021-07-28  8:26 UTC (permalink / raw)
  To: Benjamin Herrenschmidt, Michael Ellerman, Paul Mackerras,
	Christophe Leroy
  Cc: Derek Parker, Dmitrii Okunev, linuxppc-dev

Dear Benjamin,


Am 28.07.21 um 01:14 schrieb Benjamin Herrenschmidt:
> On Tue, 2021-07-27 at 10:45 +0200, Paul Menzel wrote:

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

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

* Re: Possible regression by ab037dd87a2f (powerpc/vdso: Switch VDSO to generic C implementation.)
  2021-07-28  8:26   ` Paul Menzel
@ 2021-07-28 12:43     ` Michael Ellerman
  2021-07-28 12:53       ` Paul Menzel
       [not found]       ` <OF44F7146F.67A4C1C2-ON00258720.004DBF64-00258720.004FCFCC@ibm.com>
  0 siblings, 2 replies; 14+ messages in thread
From: Michael Ellerman @ 2021-07-28 12:43 UTC (permalink / raw)
  To: Paul Menzel, Benjamin Herrenschmidt, Paul Mackerras, Christophe Leroy
  Cc: Derek Parker, Dmitrii Okunev, murp, linuxppc-dev, laboger

Paul Menzel <pmenzel@molgen.mpg.de> writes:
> Am 28.07.21 um 01:14 schrieb Benjamin Herrenschmidt:
>> On Tue, 2021-07-27 at 10:45 +0200, Paul Menzel wrote:
>
>>> 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

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

* Re: Possible regression by ab037dd87a2f (powerpc/vdso: Switch VDSO to generic C implementation.)
  2021-07-28 12:43     ` Michael Ellerman
@ 2021-07-28 12:53       ` Paul Menzel
  2021-07-29  7:41         ` Michael Ellerman
       [not found]         ` <875ywt1s9r.fsf__45665.8238823124$1627544516$gmane$org@mpe.ellerman.id.au>
       [not found]       ` <OF44F7146F.67A4C1C2-ON00258720.004DBF64-00258720.004FCFCC@ibm.com>
  1 sibling, 2 replies; 14+ messages in thread
From: Paul Menzel @ 2021-07-28 12:53 UTC (permalink / raw)
  To: Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras,
	Christophe Leroy
  Cc: Derek Parker, Dmitrii Okunev, murp, linuxppc-dev, laboger

Dear Michael,


Am 28.07.21 um 14:43 schrieb Michael Ellerman:
> Paul Menzel <pmenzel@molgen.mpg.de> writes:
>> Am 28.07.21 um 01:14 schrieb Benjamin Herrenschmidt:
>>> On Tue, 2021-07-27 at 10:45 +0200, Paul Menzel wrote:
>>
>>>> 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

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

* Re: Possible regression by ab037dd87a2f (powerpc/vdso: Switch VDSO to generic C implementation.)
  2021-07-28 12:53       ` Paul Menzel
@ 2021-07-29  7:41         ` Michael Ellerman
  2021-07-29  8:33           ` Paul Menzel
       [not found]         ` <875ywt1s9r.fsf__45665.8238823124$1627544516$gmane$org@mpe.ellerman.id.au>
  1 sibling, 1 reply; 14+ messages in thread
From: Michael Ellerman @ 2021-07-29  7:41 UTC (permalink / raw)
  To: Paul Menzel, Benjamin Herrenschmidt, Paul Mackerras, Christophe Leroy
  Cc: Derek Parker, Dmitrii Okunev, murp, linuxppc-dev, laboger

Paul Menzel <pmenzel@molgen.mpg.de> writes:

> Dear Michael,
>
>
> Am 28.07.21 um 14:43 schrieb Michael Ellerman:
>> Paul Menzel <pmenzel@molgen.mpg.de> writes:
>>> Am 28.07.21 um 01:14 schrieb Benjamin Herrenschmidt:
>>>> On Tue, 2021-07-27 at 10:45 +0200, Paul Menzel wrote:
>>>
>>>>> 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.

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


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

* Re: Possible regression by ab037dd87a2f (powerpc/vdso: Switch VDSO to generic C implementation.)
  2021-07-29  7:41         ` Michael Ellerman
@ 2021-07-29  8:33           ` Paul Menzel
  2021-07-29 12:58             ` Michael Ellerman
  0 siblings, 1 reply; 14+ messages in thread
From: Paul Menzel @ 2021-07-29  8:33 UTC (permalink / raw)
  To: Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras,
	Christophe Leroy
  Cc: Derek Parker, Dmitrii Okunev, murp, linuxppc-dev, laboger

Dear Michael,


Am 29.07.21 um 09:41 schrieb Michael Ellerman:
> Paul Menzel <pmenzel@molgen.mpg.de> writes:

>> Am 28.07.21 um 14:43 schrieb Michael Ellerman:
>>> Paul Menzel <pmenzel@molgen.mpg.de> writes:
>>>> Am 28.07.21 um 01:14 schrieb Benjamin Herrenschmidt:
>>>>> On Tue, 2021-07-27 at 10:45 +0200, Paul Menzel wrote:
>>>>
>>>>>> 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.
> 
> 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).

> 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 <pmenzel@molgen.mpg.de>

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


Kind regards,

Paul

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

* Re: Possible regression by ab037dd87a2f (powerpc/vdso: Switch VDSO to generic C implementation.)
       [not found]         ` <875ywt1s9r.fsf__45665.8238823124$1627544516$gmane$org@mpe.ellerman.id.au>
@ 2021-07-29  8:48           ` Andreas Schwab
  2021-08-02  6:04             ` Michael Ellerman
  0 siblings, 1 reply; 14+ messages in thread
From: Andreas Schwab @ 2021-07-29  8:48 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: Derek Parker, Paul Menzel, laboger, Dmitrii Okunev, murp,
	Paul Mackerras, linuxppc-dev

On Jul 29 2021, Michael Ellerman wrote:

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

Yes, only go1.14 and later are affected.

https://build.opensuse.org/package/live_build_log/openSUSE:Factory:PowerPC/go1.13/standard/ppc64
https://build.opensuse.org/package/live_build_log/openSUSE:Factory:PowerPC/go1.14/standard/ppc64

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

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

* Re: Possible regression by ab037dd87a2f (powerpc/vdso: Switch VDSO to generic C implementation.)
  2021-07-29  8:33           ` Paul Menzel
@ 2021-07-29 12:58             ` Michael Ellerman
  0 siblings, 0 replies; 14+ messages in thread
From: Michael Ellerman @ 2021-07-29 12:58 UTC (permalink / raw)
  To: Paul Menzel, Benjamin Herrenschmidt, Paul Mackerras, Christophe Leroy
  Cc: Derek Parker, Dmitrii Okunev, murp, linuxppc-dev, laboger

Paul Menzel <pmenzel@molgen.mpg.de> writes:
> Am 29.07.21 um 09:41 schrieb Michael Ellerman:
>> Paul Menzel <pmenzel@molgen.mpg.de> writes:
>
>>> Am 28.07.21 um 14:43 schrieb Michael Ellerman:
>>>> Paul Menzel <pmenzel@molgen.mpg.de> writes:
>>>>> Am 28.07.21 um 01:14 schrieb Benjamin Herrenschmidt:
>>>>>> On Tue, 2021-07-27 at 10:45 +0200, Paul Menzel wrote:
>>>>>
>>>>>>> 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.
>> 
>> 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.

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

>> +# 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 <pmenzel@molgen.mpg.de>

Thanks.

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

Yep.

cheers

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

* Re: Possible regression by ab037dd87a2f (powerpc/vdso: Switch VDSO to generic C implementation.)
  2021-07-29  8:48           ` Andreas Schwab
@ 2021-08-02  6:04             ` Michael Ellerman
  0 siblings, 0 replies; 14+ messages in thread
From: Michael Ellerman @ 2021-08-02  6:04 UTC (permalink / raw)
  To: Andreas Schwab
  Cc: Derek Parker, Paul Menzel, laboger, Dmitrii Okunev, murp,
	Paul Mackerras, linuxppc-dev

Andreas Schwab <schwab@linux-m68k.org> writes:
> On Jul 29 2021, Michael Ellerman wrote:
>
>> 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?
>
> Yes, only go1.14 and later are affected.
>
> https://build.opensuse.org/package/live_build_log/openSUSE:Factory:PowerPC/go1.13/standard/ppc64
> https://build.opensuse.org/package/live_build_log/openSUSE:Factory:PowerPC/go1.14/standard/ppc64

Thanks. That helps explain why I didn't see it, my test boxes have 1.13 installed.

cheers

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

* RE: Possible regression by ab037dd87a2f (powerpc/vdso: Switch VDSO to generic C implementation.)
       [not found]       ` <OF44F7146F.67A4C1C2-ON00258720.004DBF64-00258720.004FCFCC@ibm.com>
@ 2021-08-02  6:18         ` Michael Ellerman
  2021-08-02 12:48           ` Benjamin Herrenschmidt
                             ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Michael Ellerman @ 2021-08-02  6:18 UTC (permalink / raw)
  To: Paul Murphy, pmenzel
  Cc: parkerderek86, laboger, xaionaro, paulus, murphyp, linuxppc-dev

"Paul Murphy" <murp@ibm.com> writes:
>  
> (My apologies for however IBM's email client munges this)
>
>> 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.
>  
>  
> We have backports primed and ready for the next point release. They
> are waiting on the release manager to cherrypick them.

OK good, that is still the correct fix in the long run.

> I think we were aware that our VDSO usage may have exploited some
> peculiarities in how the ppc64 version was constructed (i.e hand
> written assembly which just didn't happen to clobber R30).

Yeah I was "somewhat surprised" that Go thought it could use r30 like
that across a VDSO call :D

But to be fair the ABI of the VDSO has always been a little fishy,
because the entry points pretend to be a transparent wrapper around
system calls, but then in a case like this are not.

> Go up to this point has only used the vdso function __kernel_clock_gettime; it
> is the only entry point which would need to explicitly avoid R30 for
> Go's sake.

I thought about limiting the workaround to just that code, but it seemed
silly and likely to come back to bite us. Once the compilers decides to
spill a non-volatile there are plenty of other ones to choose from.

cheers

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

* Re: Possible regression by ab037dd87a2f (powerpc/vdso: Switch VDSO to generic C implementation.)
  2021-08-02  6:18         ` Michael Ellerman
@ 2021-08-02 12:48           ` Benjamin Herrenschmidt
  2021-08-02 18:07           ` Segher Boessenkool
  2021-08-02 18:14           ` Lynn Boger
  2 siblings, 0 replies; 14+ messages in thread
From: Benjamin Herrenschmidt @ 2021-08-02 12:48 UTC (permalink / raw)
  To: Michael Ellerman, Paul Murphy, pmenzel
  Cc: parkerderek86, laboger, xaionaro, paulus, murphyp, linuxppc-dev

On Mon, 2021-08-02 at 16:18 +1000, Michael Ellerman wrote:
> 
> But to be fair the ABI of the VDSO has always been a little fishy,
> 
> because the entry points pretend to be a transparent wrapper around
> 
> system calls, but then in a case like this are not.

This is somewhat debatable :-) If your perspective is from an
application, whether your wrapper is glibc, the vdso or *** knows what
else, you can't make assumptions about the state of the registers on a
signal hitting somewhere in your call chain other than what's
guanranteed by the ABI overall (ie, TLS etc...).

Nowhere was it written that a VDSO call behaved strictly like an sc
instruction :-)
> 
> > Go up to this point has only used the vdso function __kernel_clock_gettime; it
> > is the only entry point which would need to explicitly avoid R30 for
> > Go's sake.
> 
> 
> I thought about limiting the workaround to just that code, but it seemed
> silly and likely to come back to bite us. Once the compilers decides to
> spill a non-volatile there are plenty of other ones to choose from.

Yeah fine graining this is a waste of time, I agree. Just stick with
fixing r30 for the whole vdso, it won't actually hurt, just make sure
this is somewhat documented as to why we do it (I don't remember what
you patch does off hand, I assume at least your git commit has all the
data, a comment near the offending line in the Makefile might be good
too).

Cheers,
Ben.



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

* Re: Possible regression by ab037dd87a2f (powerpc/vdso: Switch VDSO to generic C implementation.)
  2021-08-02  6:18         ` Michael Ellerman
  2021-08-02 12:48           ` Benjamin Herrenschmidt
@ 2021-08-02 18:07           ` Segher Boessenkool
  2021-08-02 18:14           ` Lynn Boger
  2 siblings, 0 replies; 14+ messages in thread
From: Segher Boessenkool @ 2021-08-02 18:07 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: parkerderek86, pmenzel, laboger, xaionaro, Paul Murphy, paulus,
	murphyp, linuxppc-dev

On Mon, Aug 02, 2021 at 04:18:58PM +1000, Michael Ellerman wrote:
> > Go up to this point has only used the vdso function __kernel_clock_gettime; it
> > is the only entry point which would need to explicitly avoid R30 for
> > Go's sake.
> 
> I thought about limiting the workaround to just that code, but it seemed
> silly and likely to come back to bite us. Once the compilers decides to
> spill a non-volatile there are plenty of other ones to choose from.

It can be cheaper to spill N..31 consecutively, using stmw for example.
For 64-bit Power implementations it doesn't currently make any
difference.  Since none of this will be inlined it doesn't have any real
impact.

(This also happens with -m32 -fpic, which always sets GPR30 as fixed, it
is the offset table register.  With those flags -ffixed-r30 doesn't do
anything btw (r30 already *is* a fixed function register), and this will
not work with a Go that clobbers r30.  But this is academic :-) )


Segher

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

* Re: Possible regression by ab037dd87a2f (powerpc/vdso: Switch VDSO to generic C implementation.)
  2021-08-02  6:18         ` Michael Ellerman
  2021-08-02 12:48           ` Benjamin Herrenschmidt
  2021-08-02 18:07           ` Segher Boessenkool
@ 2021-08-02 18:14           ` Lynn Boger
  2 siblings, 0 replies; 14+ messages in thread
From: Lynn Boger @ 2021-08-02 18:14 UTC (permalink / raw)
  To: Michael Ellerman, Paul Murphy, pmenzel
  Cc: parkerderek86, xaionaro, paulus, murphyp, linuxppc-dev

Fixes will be in Go 1.16.7 and 1.15.15. Backports are no longer being 
done for Go 1.14.

On 8/2/21 1:18 AM, Michael Ellerman wrote:
> "Paul Murphy" <murp@ibm.com> writes:
>>   
>> (My apologies for however IBM's email client munges this)
>>
>>> 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.
>>   
>>   
>> We have backports primed and ready for the next point release. They
>> are waiting on the release manager to cherrypick them.
> OK good, that is still the correct fix in the long run.
>
>> I think we were aware that our VDSO usage may have exploited some
>> peculiarities in how the ppc64 version was constructed (i.e hand
>> written assembly which just didn't happen to clobber R30).
> Yeah I was "somewhat surprised" that Go thought it could use r30 like
> that across a VDSO call :D
>
> But to be fair the ABI of the VDSO has always been a little fishy,
> because the entry points pretend to be a transparent wrapper around
> system calls, but then in a case like this are not.
>
>> Go up to this point has only used the vdso function __kernel_clock_gettime; it
>> is the only entry point which would need to explicitly avoid R30 for
>> Go's sake.
> I thought about limiting the workaround to just that code, but it seemed
> silly and likely to come back to bite us. Once the compilers decides to
> spill a non-volatile there are plenty of other ones to choose from.
>
> cheers

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

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

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-27  8:45 Possible regression by ab037dd87a2f (powerpc/vdso: Switch VDSO to generic C implementation.) Paul Menzel
2021-07-27 23:14 ` Benjamin Herrenschmidt
2021-07-28  8:26   ` Paul Menzel
2021-07-28 12:43     ` Michael Ellerman
2021-07-28 12:53       ` Paul Menzel
2021-07-29  7:41         ` Michael Ellerman
2021-07-29  8:33           ` Paul Menzel
2021-07-29 12:58             ` Michael Ellerman
     [not found]         ` <875ywt1s9r.fsf__45665.8238823124$1627544516$gmane$org@mpe.ellerman.id.au>
2021-07-29  8:48           ` Andreas Schwab
2021-08-02  6:04             ` Michael Ellerman
     [not found]       ` <OF44F7146F.67A4C1C2-ON00258720.004DBF64-00258720.004FCFCC@ibm.com>
2021-08-02  6:18         ` Michael Ellerman
2021-08-02 12:48           ` Benjamin Herrenschmidt
2021-08-02 18:07           ` Segher Boessenkool
2021-08-02 18:14           ` Lynn Boger

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.