* [PATCH] y2038: Remove newstat family from default syscall set
2018-09-06 9:45 ` Palmer Dabbelt
@ 2018-09-06 10:37 ` Arnd Bergmann
2018-09-06 13:24 ` Guenter Roeck
1 sibling, 0 replies; 4+ 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] 4+ messages in thread* [PATCH] y2038: Remove newstat family from default syscall set
2018-09-06 9:45 ` Palmer Dabbelt
2018-09-06 10:37 ` Arnd Bergmann
@ 2018-09-06 13:24 ` Guenter Roeck
1 sibling, 0 replies; 4+ 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] 4+ messages in thread