Re: [RFC PATCH v2 5/5] powerpc/syscalls: Allow none instead of sys_ni_syscall
Hi Arnd, On Wed, 16 Jan 2019 at 19:23, Arnd Bergmann wrote: > > On Wed, Jan 16, 2019 at 2:27 PM Michael Ellerman wrote: > > @@ -24,28 +24,28 @@ > > 14 common mknod sys_mknod > > 15 common chmod sys_chmod > > 16 common lchown sys_lchown > > -17 common break sys_ni_syscall > > -18 32 oldstat sys_stat > > sys_ni_syscall > > -18 64 oldstat sys_ni_syscall > > +17 common break none > > +18 32 oldstat sys_stat > > none > > +18 64 oldstat none > > The '64 oldstat' line can simply get dropped here, it has no value > (I failed to notice this earlier). The initial requirement is to replace uapi and syscalltbl file with the script as it is. If I'm right, for oldstat has uapi header and syscalltbl entry is sys_ni_syscall. So the above change will replace uapi and syscalltbl as it is. We'll do a cleanup for all 10 architectures, so that time we can remove this (and similer) entries. Thanks Firoz
Re: [RFC PATCH v2 5/5] powerpc/syscalls: Allow none instead of sys_ni_syscall
On Thu, Jan 17, 2019 at 11:35 AM Michael Ellerman wrote: > > Hi Arnd, > > Arnd Bergmann writes: > > On Wed, Jan 16, 2019 at 2:27 PM Michael Ellerman > > wrote: > >> > >> sys_ni_syscall is the "not-implemented" syscall syscall, which just > >> returns -ENOSYS. > >> > >> But unless you know that it's not obvious what it does, and even if > >> you do know what it means it doesn't stand out that well from other > >> real syscalls. > >> > >> So teach the scripts to treat "none" as a synonym for > >> "sys_ni_syscall". This makes the table more readable. > > > > Hmm, this actually breaks the proposed script to find bugs > > in the compat handling, i.e. detecting those that have no > > compat handler but only a native one. > > I don't understand how? It just makes none an alias for sys_ni_syscall, > so surely the worse case is that script will need to do the reverse > transformation? I mean it's broken to have a common script when architectures do it differently. It would be fine if you changed all architectures at the same time though. > > For break, i.e. a syscall number without any implementation, > > we use a different syntax on x86 (leaving out the sys_* entirely), > > and on s390 (using '-', which is visually better than 'none' IMHO). > > Except a blank compat syscall doesn't mean the syscall doesn't exist for > compat tasks, it means they get the non-compat entry point. So blank or > '-' are not explicit enough IMO because the script might have some > default logic which you can't see by looking at the table. > > "none" is pretty explicit I thought. Possibly better is a literal > "ENOSYS", which stands out well and should be obvious to new comers. ENOSYS is fine with me as well, but most importantly please don't make powerpc different from the other ones for a matter of personal preference. Whatever you want to change it to, please make the patch change all syscall.tbl files at once, and explain in the patch why we should do this across all architectures, then see if anyone objects. > > We might also just remove those entirely across all architectures. > > Some have already done this, and some have done it partially. > > I can only see a couple of syscalls that got removed in the entire > > git history (set_zone_reclaim, nfsservctl, vm86, timerfd), any other > > ones are now literally pre-historic, and presumably nobody would > > miss the macros when building a program that has no chance to > > run on any kernel since at least 2.6.12. > > I don't see the benefit, a single missing #define could be a build break > for some random pieces of software out there. It certainly could, the question is whether that is a bad thing or not. Once again, I think it's most important to be consistent across architectures, and either define them everywhere or nowhere so we don't end up with applications that are only broken on less common architectures but work on fine on others. Most of these only still exist in a few architectures anyway: $ git grep sys_ni_syscall arch/{*/,}*/*/syscall*.tbl | grep -v '\<\(osf\|available\|reserved\|unused\)' | awk '{ print $3; }' | sort | uniq -c 7 afs_syscall 3 break 8 create_module 1 dipc 1 exec_with_loader 1 fadvise64_64 3 ftime 8 get_kernel_syms 1 get_mempolicy 7 getpmsg 3 getrlimit 1 get_thread_area 3 gtty 3 idle 3 ioperm 3 iopl 1 ipc 1 kern_features 1 kexec_load 3 lock 1 mbind 1 migrate_pages 3 modify_ldt 3 mpx 1 multiplexer 11 nfsservctl 1 ni_syscall 3 oldfstat 3 oldlstat 3 oldolduname 3 oldstat 3 olduname 3 prof 3 profil 7 putpmsg 8 query_module 3 readdir 4 select 1 set_mempolicy 1 set_thread_area 3 sigaction 1 sigaltstack 1 signal 2 sigpending 2 sigprocmask 3 sigreturn 3 sigsuspend 1 spill 3 stty 1 swapcontext 2 switch_endian 3 sys_debug_setcontext 5 timerfd 2 tuxcall 3 ulimit 2 umount 1 uselib 4 vm86 2 vm86old 6 vserver 1 xtensa I would guess that there are enough syscall names here that are more likely to cause problems if the macro is defined then when it is missing, e.g. a common method in glibc is to check for the older symbol first: int __renameat (int oldfd, const char *old, int newfd, const char *new) { #ifdef __NR_renameat return INLINE_SYSCALL_CALL (renameat, oldfd, old, newfd, new); #else return INLINE_SYSCALL_CALL (renameat2, oldfd, old, newfd, new, 0); #endif } and this breaks if __NR_renameat is sys_ni_syscall. I could not find any such example that is actually broken with glibc today (presumably others have checked before me), but other software may do similar things. > > For 32-bit oldstat, I'd argue that this should actually get fixed by adding > > the compat syscall logic. I think this
Re: [RFC PATCH v2 5/5] powerpc/syscalls: Allow none instead of sys_ni_syscall
Hi Arnd, Arnd Bergmann writes: > On Wed, Jan 16, 2019 at 2:27 PM Michael Ellerman wrote: >> >> sys_ni_syscall is the "not-implemented" syscall syscall, which just >> returns -ENOSYS. >> >> But unless you know that it's not obvious what it does, and even if >> you do know what it means it doesn't stand out that well from other >> real syscalls. >> >> So teach the scripts to treat "none" as a synonym for >> "sys_ni_syscall". This makes the table more readable. > > Hmm, this actually breaks the proposed script to find bugs > in the compat handling, i.e. detecting those that have no > compat handler but only a native one. I don't understand how? It just makes none an alias for sys_ni_syscall, so surely the worse case is that script will need to do the reverse transformation? >> diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl >> b/arch/powerpc/kernel/syscalls/syscall.tbl >> index c5907a2dbc86..988a7e29245f 100644 >> --- a/arch/powerpc/kernel/syscalls/syscall.tbl >> +++ b/arch/powerpc/kernel/syscalls/syscall.tbl >> @@ -24,28 +24,28 @@ >> 14 common mknod sys_mknod >> 15 common chmod sys_chmod >> 16 common lchown sys_lchown >> -17 common break sys_ni_syscall >> -18 32 oldstat sys_stat >>sys_ni_syscall >> -18 64 oldstat sys_ni_syscall >> +17 common break none >> +18 32 oldstat sys_stat >>none >> +18 64 oldstat none > > The '64 oldstat' line can simply get dropped here, it has no value > (I failed to notice this earlier). It does add value. It causes the syscall number to be defined in unistd.h (now unistd_64.h). If you remove it then that syscall number is no longer defined which changes the uapi header and could break something. Sure arguably it shouldn't be defined, and it's old etc. but it was previously defined, so removing it seems risky. > For break, i.e. a syscall number without any implementation, > we use a different syntax on x86 (leaving out the sys_* entirely), > and on s390 (using '-', which is visually better than 'none' IMHO). Except a blank compat syscall doesn't mean the syscall doesn't exist for compat tasks, it means they get the non-compat entry point. So blank or '-' are not explicit enough IMO because the script might have some default logic which you can't see by looking at the table. "none" is pretty explicit I thought. Possibly better is a literal "ENOSYS", which stands out well and should be obvious to new comers. > We might also just remove those entirely across all architectures. > Some have already done this, and some have done it partially. > I can only see a couple of syscalls that got removed in the entire > git history (set_zone_reclaim, nfsservctl, vm86, timerfd), any other > ones are now literally pre-historic, and presumably nobody would > miss the macros when building a program that has no chance to > run on any kernel since at least 2.6.12. I don't see the benefit, a single missing #define could be a build break for some random pieces of software out there. > For 32-bit oldstat, I'd argue that this should actually get fixed by adding > the compat syscall logic. I think this was discussed when Firoz > first posted his patches. Something like this: I'm not clear why we would do that? If there were programs out there that wanted oldstat in compat mode surely we would have got a bug report by now. So this just wires up a syscall that no one will ever use? cheers > diff --git a/arch/powerpc/include/asm/unistd.h > b/arch/powerpc/include/asm/unistd.h > index f44dbc65e38e..d954c2fc4e2f 100644 > --- a/arch/powerpc/include/asm/unistd.h > +++ b/arch/powerpc/include/asm/unistd.h > @@ -41,9 +41,7 @@ > #define __ARCH_WANT_SYS_OLDUMOUNT > #define __ARCH_WANT_SYS_SIGPENDING > #define __ARCH_WANT_SYS_SIGPROCMASK > -#ifdef CONFIG_PPC32 > #define __ARCH_WANT_OLD_STAT > -#endif > #ifdef CONFIG_PPC64 > #define __ARCH_WANT_SYS_TIME > #define __ARCH_WANT_SYS_UTIME > diff --git a/arch/powerpc/include/uapi/asm/stat.h > b/arch/powerpc/include/uapi/asm/stat.h > index afd25f2ff4e8..8331b350c12b 100644 > --- a/arch/powerpc/include/uapi/asm/stat.h > +++ b/arch/powerpc/include/uapi/asm/stat.h > @@ -11,7 +11,7 @@ > > #define STAT_HAVE_NSEC 1 > > -#ifndef __powerpc64__ > +#if defined(__KERNEL__) || !defined(__powerpc64__) > struct __old_kernel_stat { > unsigned short st_dev; > unsigned short st_ino; > @@ -25,7 +25,7 @@ struct __old_kernel_stat { > unsigned long st_mtime; > unsigned long st_ctime; > }; > -#endif /* !__powerpc64__ */ > +#endif /* __KERNEL__ || !__powerpc64__ */ > > struct stat { > unsigned long st_dev; > diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl >
Re: [RFC PATCH v2 5/5] powerpc/syscalls: Allow none instead of sys_ni_syscall
On Wed, Jan 16, 2019 at 2:27 PM Michael Ellerman wrote: > > sys_ni_syscall is the "not-implemented" syscall syscall, which just > returns -ENOSYS. > > But unless you know that it's not obvious what it does, and even if > you do know what it means it doesn't stand out that well from other > real syscalls. > > So teach the scripts to treat "none" as a synonym for > "sys_ni_syscall". This makes the table more readable. Hmm, this actually breaks the proposed script to find bugs in the compat handling, i.e. detecting those that have no compat handler but only a native one. > diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl > b/arch/powerpc/kernel/syscalls/syscall.tbl > index c5907a2dbc86..988a7e29245f 100644 > --- a/arch/powerpc/kernel/syscalls/syscall.tbl > +++ b/arch/powerpc/kernel/syscalls/syscall.tbl > @@ -24,28 +24,28 @@ > 14 common mknod sys_mknod > 15 common chmod sys_chmod > 16 common lchown sys_lchown > -17 common break sys_ni_syscall > -18 32 oldstat sys_stat > sys_ni_syscall > -18 64 oldstat sys_ni_syscall > +17 common break none > +18 32 oldstat sys_stat > none > +18 64 oldstat none The '64 oldstat' line can simply get dropped here, it has no value (I failed to notice this earlier). For break, i.e. a syscall number without any implementation, we use a different syntax on x86 (leaving out the sys_* entirely), and on s390 (using '-', which is visually better than 'none' IMHO). We might also just remove those entirely across all architectures. Some have already done this, and some have done it partially. I can only see a couple of syscalls that got removed in the entire git history (set_zone_reclaim, nfsservctl, vm86, timerfd), any other ones are now literally pre-historic, and presumably nobody would miss the macros when building a program that has no chance to run on any kernel since at least 2.6.12. For 32-bit oldstat, I'd argue that this should actually get fixed by adding the compat syscall logic. I think this was discussed when Firoz first posted his patches. Something like this: diff --git a/arch/powerpc/include/asm/unistd.h b/arch/powerpc/include/asm/unistd.h index f44dbc65e38e..d954c2fc4e2f 100644 --- a/arch/powerpc/include/asm/unistd.h +++ b/arch/powerpc/include/asm/unistd.h @@ -41,9 +41,7 @@ #define __ARCH_WANT_SYS_OLDUMOUNT #define __ARCH_WANT_SYS_SIGPENDING #define __ARCH_WANT_SYS_SIGPROCMASK -#ifdef CONFIG_PPC32 #define __ARCH_WANT_OLD_STAT -#endif #ifdef CONFIG_PPC64 #define __ARCH_WANT_SYS_TIME #define __ARCH_WANT_SYS_UTIME diff --git a/arch/powerpc/include/uapi/asm/stat.h b/arch/powerpc/include/uapi/asm/stat.h index afd25f2ff4e8..8331b350c12b 100644 --- a/arch/powerpc/include/uapi/asm/stat.h +++ b/arch/powerpc/include/uapi/asm/stat.h @@ -11,7 +11,7 @@ #define STAT_HAVE_NSEC 1 -#ifndef __powerpc64__ +#if defined(__KERNEL__) || !defined(__powerpc64__) struct __old_kernel_stat { unsigned short st_dev; unsigned short st_ino; @@ -25,7 +25,7 @@ struct __old_kernel_stat { unsigned long st_mtime; unsigned long st_ctime; }; -#endif /* !__powerpc64__ */ +#endif /* __KERNEL__ || !__powerpc64__ */ struct stat { unsigned long st_dev; diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl b/arch/powerpc/kernel/syscalls/syscall.tbl index 740dc9dbf689..cd85718c7039 100644 --- a/arch/powerpc/kernel/syscalls/syscall.tbl +++ b/arch/powerpc/kernel/syscalls/syscall.tbl @@ -27,7 +27,7 @@ 15 common chmod sys_chmod 16 common lchown sys_lchown 17 common break sys_ni_syscall -18 32 oldstat sys_stat sys_ni_syscall +18 32 oldstat sys_stat 18 64 oldstat sys_ni_syscall 18 spu oldstat sys_ni_syscall 19 common lseek sys_lseek compat_sys_lseek @@ -43,7 +43,7 @@ 25 spu stime sys_stime 26 nospu ptrace sys_ptrace compat_sys_ptrace 27 common alarm sys_alarm -28 32 oldfstatsys_fstat sys_ni_syscall +28 32 oldfstatsys_fstat 28 64 oldfstatsys_ni_syscall 28 spu oldfstatsys_ni_syscall 29 nospu pause sys_pause @@ -114,7 +114,7 @@ 82 64 select sys_ni_syscall 82 spu select sys_ni_syscall 83 common symlink