* [PATCH] y2038: Remove newstat family from default syscall set
@ 2018-09-06 9:45 ` Palmer Dabbelt
0 siblings, 0 replies; 8+ messages in thread
From: Palmer Dabbelt @ 2018-09-06 9:45 UTC (permalink / raw)
To: linux-riscv
On Sat, 01 Sep 2018 10:43:53 PDT (-0700), linux at roeck-us.net wrote:
> Hi Arnd,
>
> On Fri, Apr 13, 2018 at 11:50:12AM +0200, Arnd Bergmann wrote:
>> We have four generations of stat() syscalls:
>> - the oldstat syscalls that are only used on the older architectures
>> - the newstat family that is used on all 64-bit architectures but
>> lacked support for large files on 32-bit architectures.
>> - the stat64 family that is used mostly on 32-bit architectures to
>> replace newstat
>> - statx() to replace all of the above, adding 64-bit timestamps among
>> other things.
>>
>> We already compile stat64 only on those architectures that need it,
>> but newstat is always built, including on those that don't reference
>> it. This adds a new __ARCH_WANT_NEW_STAT symbol along the lines of
>> __ARCH_WANT_OLD_STAT and __ARCH_WANT_STAT64 to control compilation of
>> newstat. All architectures that need it use an explict define, the
>> others now get a little bit smaller, and future architecture (including
>> 64-bit targets) won't ever see it.
>>
>
> This patch causes my riscv boot tests to crash in -next
Ah, thanks for running these!
> sbin/init: error while loading shared libraries: libc.so.6: cannot stat shared object: Error 38
> Kernel panic - not syncing: Attempted to kill init! exitcode=0x00007f00
>
> The following change fixes the problem for me, but of course I have no idea
> if it is correct. Copying RISC-V maintainers for input.
>
> Guenter
>
> ---
> diff --git a/arch/riscv/include/asm/unistd.h b/arch/riscv/include/asm/unistd.h
> index 0caea01d5cca..eff7aa9aa163 100644
> --- a/arch/riscv/include/asm/unistd.h
> +++ b/arch/riscv/include/asm/unistd.h
> @@ -16,6 +16,7 @@
> * be included multiple times. See uapi/asm/syscalls.h for more info.
> */
>
> +#define __ARCH_WANT_NEW_STAT
> #define __ARCH_WANT_SYS_CLONE
> #include <uapi/asm/unistd.h>
> #include <uapi/asm/syscalls.h>
I'm afraid I'm not sure what the right thing to do here is either, but from the
patch description it does seem like we should have this guarded by an "#ifdef
CONFIG_32BIT" so we can keep it out of our 32-bit ABI (which isn't in glibc
yet, so isn't stable) in favor of statx() (or maybe stat64()?). The one
problem here is that I can't find "newstat" anywhere in glibc to verify it's
actually supposed to be part of our 64-bit ABI, though I can find a bunch of
references to "statx" that seem to indicate it's meant to be present.
That said, assuming you don't have anything wacky going on in userspace if this
breaks the ABI then it breaks the ABI, so however newstat got into a binary we
still need to keep it around. Poking around my Fedora glibc image I see
000000000009b040 <__xstat>:
9b040: e51d bnez a0,9b06e <__xstat+0x2e>
9b042: 04f00893 li a7,79
9b046: f9c00513 li a0,-100
9b04a: 4681 li a3,0
9b04c: 00000073 ecall
which seems to coorespond with sys_newfstatat, which indicates to me we should
have it in the 64-bit ABI.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] y2038: Remove newstat family from default syscall set
2018-09-06 9:45 ` Palmer Dabbelt
@ 2018-09-06 10:37 ` Arnd Bergmann
-1 siblings, 0 replies; 8+ messages in thread
From: Arnd Bergmann @ 2018-09-06 10:37 UTC (permalink / raw)
To: Palmer Dabbelt; +Cc: Guenter Roeck, aou, linux-riscv, Linux Kernel Mailing List
On Thu, Sep 6, 2018 at 11:45 AM Palmer Dabbelt <palmer@sifive.com> wrote:
> On Sat, 01 Sep 2018 10:43:53 PDT (-0700), linux@roeck-us.net wrote:
> >
> > +#define __ARCH_WANT_NEW_STAT
> > #define __ARCH_WANT_SYS_CLONE
> > #include <uapi/asm/unistd.h>
> > #include <uapi/asm/syscalls.h>
>
> I'm afraid I'm not sure what the right thing to do here is either, but from the
> patch description it does seem like we should have this guarded by an "#ifdef
> CONFIG_32BIT" so we can keep it out of our 32-bit ABI (which isn't in glibc
> yet, so isn't stable) in favor of statx() (or maybe stat64()?).
I think the patch is correct.
> The one
> problem here is that I can't find "newstat" anywhere in glibc to verify it's
> actually supposed to be part of our 64-bit ABI, though I can find a bunch of
> references to "statx" that seem to indicate it's meant to be present.
>
> That said, assuming you don't have anything wacky going on in userspace if this
> breaks the ABI then it breaks the ABI, so however newstat got into a binary we
> still need to keep it around. Poking around my Fedora glibc image I see
>
> 000000000009b040 <__xstat>:
> 9b040: e51d bnez a0,9b06e <__xstat+0x2e>
> 9b042: 04f00893 li a7,79
> 9b046: f9c00513 li a0,-100
> 9b04a: 4681 li a3,0
> 9b04c: 00000073 ecall
>
> which seems to coorespond with sys_newfstatat, which indicates to me we should
> have it in the 64-bit ABI.
In uapi/asm-generic/unistd.h we have
#if defined(__ARCH_WANT_NEW_STAT) || defined(__ARCH_WANT_STAT64)
#define __NR3264_fstatat 79
__SC_3264(__NR3264_fstatat, sys_fstatat64, sys_newfstatat)
#define __NR3264_fstat 80
__SC_3264(__NR3264_fstat, sys_fstat64, sys_newfstat)
#endif
#define __NR_newfstatat __NR3264_fstatat
#define __NR_fstat __NR3264_fstat
#if __BITS_PER_LONG == 64 && !defined(__SYSCALL_COMPAT)
#else
#define __NR_fstatat64 __NR3264_fstatat
#define __NR_fstat64 __NR3264_fstat
#endif
So in the kernel, we have two families of implementations, both
with awful historic names:
On 64-bit systems, we have __NR_newfstatat pointing to sys_newfstatat
(with a 'struct stat argument), and on 32-bit systems we have
__NR_fstatat64 pointing to sys_fstatat64 (with a struct stat64 argument).
In glibc, we have __xstat, which calls __NR_newfstatat on 64-bit systems,
and __NR_fstatat64 on 32-bit systems. The result is the same in both
cases, the only user-visible difference is the layout of the atime/mtime/ctime
fields.
Arnd
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH] y2038: Remove newstat family from default syscall set
@ 2018-09-06 10:37 ` Arnd Bergmann
0 siblings, 0 replies; 8+ messages in thread
From: Arnd Bergmann @ 2018-09-06 10:37 UTC (permalink / raw)
To: linux-riscv
On Thu, Sep 6, 2018 at 11:45 AM Palmer Dabbelt <palmer@sifive.com> wrote:
> On Sat, 01 Sep 2018 10:43:53 PDT (-0700), linux at roeck-us.net wrote:
> >
> > +#define __ARCH_WANT_NEW_STAT
> > #define __ARCH_WANT_SYS_CLONE
> > #include <uapi/asm/unistd.h>
> > #include <uapi/asm/syscalls.h>
>
> I'm afraid I'm not sure what the right thing to do here is either, but from the
> patch description it does seem like we should have this guarded by an "#ifdef
> CONFIG_32BIT" so we can keep it out of our 32-bit ABI (which isn't in glibc
> yet, so isn't stable) in favor of statx() (or maybe stat64()?).
I think the patch is correct.
> The one
> problem here is that I can't find "newstat" anywhere in glibc to verify it's
> actually supposed to be part of our 64-bit ABI, though I can find a bunch of
> references to "statx" that seem to indicate it's meant to be present.
>
> That said, assuming you don't have anything wacky going on in userspace if this
> breaks the ABI then it breaks the ABI, so however newstat got into a binary we
> still need to keep it around. Poking around my Fedora glibc image I see
>
> 000000000009b040 <__xstat>:
> 9b040: e51d bnez a0,9b06e <__xstat+0x2e>
> 9b042: 04f00893 li a7,79
> 9b046: f9c00513 li a0,-100
> 9b04a: 4681 li a3,0
> 9b04c: 00000073 ecall
>
> which seems to coorespond with sys_newfstatat, which indicates to me we should
> have it in the 64-bit ABI.
In uapi/asm-generic/unistd.h we have
#if defined(__ARCH_WANT_NEW_STAT) || defined(__ARCH_WANT_STAT64)
#define __NR3264_fstatat 79
__SC_3264(__NR3264_fstatat, sys_fstatat64, sys_newfstatat)
#define __NR3264_fstat 80
__SC_3264(__NR3264_fstat, sys_fstat64, sys_newfstat)
#endif
#define __NR_newfstatat __NR3264_fstatat
#define __NR_fstat __NR3264_fstat
#if __BITS_PER_LONG == 64 && !defined(__SYSCALL_COMPAT)
#else
#define __NR_fstatat64 __NR3264_fstatat
#define __NR_fstat64 __NR3264_fstat
#endif
So in the kernel, we have two families of implementations, both
with awful historic names:
On 64-bit systems, we have __NR_newfstatat pointing to sys_newfstatat
(with a 'struct stat argument), and on 32-bit systems we have
__NR_fstatat64 pointing to sys_fstatat64 (with a struct stat64 argument).
In glibc, we have __xstat, which calls __NR_newfstatat on 64-bit systems,
and __NR_fstatat64 on 32-bit systems. The result is the same in both
cases, the only user-visible difference is the layout of the atime/mtime/ctime
fields.
Arnd
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] y2038: Remove newstat family from default syscall set
2018-09-06 9:45 ` Palmer Dabbelt
@ 2018-09-06 13:24 ` Guenter Roeck
-1 siblings, 0 replies; 8+ messages in thread
From: Guenter Roeck @ 2018-09-06 13:24 UTC (permalink / raw)
To: Palmer Dabbelt; +Cc: Arnd Bergmann, aou, linux-riscv, linux-kernel
On 09/06/2018 02:45 AM, Palmer Dabbelt wrote:
> On Sat, 01 Sep 2018 10:43:53 PDT (-0700), linux@roeck-us.net wrote:
>> Hi Arnd,
>>
>> On Fri, Apr 13, 2018 at 11:50:12AM +0200, Arnd Bergmann wrote:
>>> We have four generations of stat() syscalls:
>>> - the oldstat syscalls that are only used on the older architectures
>>> - the newstat family that is used on all 64-bit architectures but
>>> lacked support for large files on 32-bit architectures.
>>> - the stat64 family that is used mostly on 32-bit architectures to
>>> replace newstat
>>> - statx() to replace all of the above, adding 64-bit timestamps among
>>> other things.
>>>
>>> We already compile stat64 only on those architectures that need it,
>>> but newstat is always built, including on those that don't reference
>>> it. This adds a new __ARCH_WANT_NEW_STAT symbol along the lines of
>>> __ARCH_WANT_OLD_STAT and __ARCH_WANT_STAT64 to control compilation of
>>> newstat. All architectures that need it use an explict define, the
>>> others now get a little bit smaller, and future architecture (including
>>> 64-bit targets) won't ever see it.
>>>
>>
>> This patch causes my riscv boot tests to crash in -next
>
> Ah, thanks for running these!
>
>> sbin/init: error while loading shared libraries: libc.so.6: cannot stat shared object: Error 38
>> Kernel panic - not syncing: Attempted to kill init! exitcode=0x00007f00
>>
>> The following change fixes the problem for me, but of course I have no idea
>> if it is correct. Copying RISC-V maintainers for input.
>>
>> Guenter
>>
>> ---
>> diff --git a/arch/riscv/include/asm/unistd.h b/arch/riscv/include/asm/unistd.h
>> index 0caea01d5cca..eff7aa9aa163 100644
>> --- a/arch/riscv/include/asm/unistd.h
>> +++ b/arch/riscv/include/asm/unistd.h
>> @@ -16,6 +16,7 @@
>> * be included multiple times. See uapi/asm/syscalls.h for more info.
>> */
>>
>> +#define __ARCH_WANT_NEW_STAT
>> #define __ARCH_WANT_SYS_CLONE
>> #include <uapi/asm/unistd.h>
>> #include <uapi/asm/syscalls.h>
>
> I'm afraid I'm not sure what the right thing to do here is either, but from the patch description it does seem like we should have this guarded by an "#ifdef CONFIG_32BIT" so we can keep it out of our 32-bit ABI (which isn't in glibc yet, so isn't stable) in favor of statx() (or maybe stat64()?). The one problem here is that I can't find "newstat" anywhere in glibc to verify it's actually supposed to be part of our 64-bit ABI, though I can find a bunch of references to "statx" that seem to indicate it's meant to be present.
>
> That said, assuming you don't have anything wacky going on in userspace if this breaks the ABI then it breaks the ABI, so however newstat got into a binary we still need to keep it around. Poking around my Fedora glibc image I see
>
userspace is a root file system built with buildroot (modified to support riscv64),
using glibc 2.26. Nothing wacky, sorry.
Guenter
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] y2038: Remove newstat family from default syscall set
@ 2018-09-06 13:24 ` Guenter Roeck
0 siblings, 0 replies; 8+ messages in thread
From: Guenter Roeck @ 2018-09-06 13:24 UTC (permalink / raw)
To: linux-riscv
On 09/06/2018 02:45 AM, Palmer Dabbelt wrote:
> On Sat, 01 Sep 2018 10:43:53 PDT (-0700), linux at roeck-us.net wrote:
>> Hi Arnd,
>>
>> On Fri, Apr 13, 2018 at 11:50:12AM +0200, Arnd Bergmann wrote:
>>> We have four generations of stat() syscalls:
>>> - the oldstat syscalls that are only used on the older architectures
>>> - the newstat family that is used on all 64-bit architectures but
>>> ? lacked support for large files on 32-bit architectures.
>>> - the stat64 family that is used mostly on 32-bit architectures to
>>> ? replace newstat
>>> - statx() to replace all of the above, adding 64-bit timestamps among
>>> ? other things.
>>>
>>> We already compile stat64 only on those architectures that need it,
>>> but newstat is always built, including on those that don't reference
>>> it. This adds a new __ARCH_WANT_NEW_STAT symbol along the lines of
>>> __ARCH_WANT_OLD_STAT and __ARCH_WANT_STAT64 to control compilation of
>>> newstat. All architectures that need it use an explict define, the
>>> others now get a little bit smaller, and future architecture (including
>>> 64-bit targets) won't ever see it.
>>>
>>
>> This patch causes my riscv boot tests to crash in -next
>
> Ah, thanks for running these!
>
>> sbin/init: error while loading shared libraries: libc.so.6: cannot stat shared object: Error 38
>> Kernel panic - not syncing: Attempted to kill init! exitcode=0x00007f00
>>
>> The following change fixes the problem for me, but of course I have no idea
>> if it is correct. Copying RISC-V maintainers for input.
>>
>> Guenter
>>
>> ---
>> diff --git a/arch/riscv/include/asm/unistd.h b/arch/riscv/include/asm/unistd.h
>> index 0caea01d5cca..eff7aa9aa163 100644
>> --- a/arch/riscv/include/asm/unistd.h
>> +++ b/arch/riscv/include/asm/unistd.h
>> @@ -16,6 +16,7 @@
>> ? * be included multiple times.? See uapi/asm/syscalls.h for more info.
>> ? */
>>
>> +#define __ARCH_WANT_NEW_STAT
>> ?#define __ARCH_WANT_SYS_CLONE
>> ?#include <uapi/asm/unistd.h>
>> ?#include <uapi/asm/syscalls.h>
>
> I'm afraid I'm not sure what the right thing to do here is either, but from the patch description it does seem like we should have this guarded by an "#ifdef CONFIG_32BIT" so we can keep it out of our 32-bit ABI (which isn't in glibc yet, so isn't stable) in favor of statx() (or maybe stat64()?).? The one problem here is that I can't find "newstat" anywhere in glibc to verify it's actually supposed to be part of our 64-bit ABI, though I can find a bunch of references to "statx" that seem to indicate it's meant to be present.
>
> That said, assuming you don't have anything wacky going on in userspace if this breaks the ABI then it breaks the ABI, so however newstat got into a binary we still need to keep it around.? Poking around my Fedora glibc image I see
>
userspace is a root file system built with buildroot (modified to support riscv64),
using glibc 2.26. Nothing wacky, sorry.
Guenter
^ permalink raw reply [flat|nested] 8+ messages in thread