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 [off-list ref] wrote:

Hi Arnd,

Arnd Bergmann [off-list ref] writes:

quoted

On Wed, Jan 16, 2019 at 2:27 PM Michael Ellerman [off-list ref] wrote:

quoted

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.

quoted

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.

quoted

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.

quoted

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?

compat mode is not always reliable, so it's entirely possible that
someone tried it and gave up when it didn't work right away.

My point is that we should always try to make compat mode behave
the exact same way as native mode, if there is a difference between
them, it's a bug of the emulation.

If you are sure that nothing ever enters those system calls, then
we should remove them from native 32-bit mode, otherwise we should
add them in compat mode.

      Arnd