[RFC PATCH v2 5/5] powerpc/syscalls: Allow none instead of sys_ni_syscall

Michael Ellerman mpe at ellerman.id.au
Thu Jan 17 21:35:03 AEDT 2019


Hi Arnd,

Arnd Bergmann <arnd at arndb.de> writes:
> On Wed, Jan 16, 2019 at 2:27 PM Michael Ellerman <mpe at ellerman.id.au> 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
> 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      oldfstat                        sys_fstat           sys_ni_syscall
> +28     32      oldfstat                        sys_fstat
>  28     64      oldfstat                        sys_ni_syscall
>  28     spu     oldfstat                        sys_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                         sys_symlink
> -84     32      oldlstat                        sys_lstat           sys_ni_syscall
> +84     32      oldlstat                        sys_lstat
>  84     64      oldlstat                        sys_ni_syscall
>  84     spu     oldlstat                        sys_ni_syscall
>  85     common  readlink                        sys_readlink


More information about the Linuxppc-dev mailing list