[PATCH 05/13] riscv: compat: syscall: Add compat_sys_call_table implementation

[Arnd Bergmann]

On Wed, Dec 22, 2021 at 1:43 PM Guo Ren <guoren at kernel.org> wrote:
>
> On Wed, Dec 22, 2021 at 2:15 AM Arnd Bergmann <arnd at arndb.de> wrote:
> >
> > On Tue, Dec 21, 2021 at 5:35 PM <guoren at kernel.org> wrote:
> > >
> > > From: Guo Ren <guoren at linux.alibaba.com>
> > >
> > > Implement compat_syscall_table.c with compat_sys_call_table & fixup
> > > system call such as truncate64,pread64,fallocate which need two
> > > regs to indicate 64bit-arg (copied from arm64).
> > >
> > > Signed-off-by: Guo Ren <guoren at linux.alibaba.com>
> > > ---
> > >  arch/riscv/include/asm/syscall.h         |  3 +
> > >  arch/riscv/kernel/compat_syscall_table.c | 84 ++++++++++++++++++++++++
> >
> > Same here, I think most of these should go next to the actual syscalls, with the
> > duplicates removed from other platforms,
> Agree, I will try that next version.
>
> >
> > > +#define __SYSCALL_COMPAT
> > > +#undef __LP64__
> >
> > What is the #undef for?
>
> See arch/riscv/include/uapi/asm/unistd.h:
>
> #ifdef __LP64__
> #define __ARCH_WANT_NEW_STAT
> #define __ARCH_WANT_SET_GET_RLIMIT
> #endif /* __LP64__ */

Ok, in this case I would recommend changing that #ifdef to
check for __SYSCALL_COMPAT instead, as removing the
__LP64__ define may cause other unintended changes.


> > > +SYSCALL_DEFINE6(mmap2, unsigned long, addr, unsigned long, len,
> > > +       unsigned long, prot, unsigned long, flags,
> > > +       unsigned long, fd, unsigned long, offset)
> > > +{
> > > +       if ((prot & PROT_WRITE) && (prot & PROT_EXEC))
> > > +               if (unlikely(!(prot & PROT_READ)))
> > > +                       return -EINVAL;
> > > +
> > > +       return ksys_mmap_pgoff(addr, len, prot, flags, fd, offset);
> > > +}
> >
> > This is one that we may have to deal with separately, introducing
> > sys_mmap_pgoff() was a mistake in my opinion, and we should just have
>
> #if __BITS_PER_LONG == 32 || defined(__SYSCALL_COMPAT)
> #define __SC_3264(_nr, _32, _64) __SYSCALL(_nr, _32)
> #else
> #define __SC_3264(_nr, _32, _64) __SYSCALL(_nr, _64)
> #endif
>
> #define __NR3264_mmap 222
> __SC_3264(__NR3264_mmap, sys_mmap2, sys_mmap)
>
> > added a sys_mmap2() for all architectures that don't explicitly override it.
> That should be another patch, right? Let's keep it here.

Right, I think the patch would be a nice cleanup, but it appears that
riscv is among the few architectures that have defined their own
nonstandard mmap2() syscall after all, despite using the standard
name for the entry point. Not sure how this slipped past my original
review, but it certainly can't be changed now.

It also means that you need to change your implementation of
compat_sys_mmap2() to match the version from
arch/riscv/kernel/sys_riscv.c, rather than the version that
everyone else has. Maybe leave it there and change the
#ifdef to build mmap2 for both native rv32 and compat
mode.

FWIW, this is what a conversion from mmap_pgoff() to mmap2()
would look like, I think this is an overall win, but it's entirely
unrelated to your series now: https://pastebin.com/QtF55dn4
(I'm sure I got some details wrong, at least it needs some
#ifdef checks).

       Arnd