Re: [PATCH 1/2] x86/x32: Use __x64 prefix for X32 compat syscalls

[Brian Gerst]

On Tue, Jun 16, 2020 at 12:49 PM Andy Lutomirski <luto@xxxxxxxxxx> wrote:
>
> On Tue, Jun 16, 2020 at 7:23 AM Brian Gerst <brgerst@xxxxxxxxx> wrote:
> >
> > The ABI prefix for syscalls specifies the argument register mapping, so
> > there is no specific reason to continue using the __x32 prefix for the
> > compat syscalls.  This change will allow using native syscalls in the X32
> > specific portion of the syscall table.
>
> Okay, I realize that the x86 syscall machinery is held together by
> duct tape and a lot of luck, but:
>
> >
> > Signed-off-by: Brian Gerst <brgerst@xxxxxxxxx>
> > ---
> >  arch/x86/entry/syscall_x32.c           |  8 +++-----
> >  arch/x86/include/asm/syscall_wrapper.h | 10 +++++-----
> >  2 files changed, 8 insertions(+), 10 deletions(-)
> >
> > diff --git a/arch/x86/entry/syscall_x32.c b/arch/x86/entry/syscall_x32.c
> > index 3d8d70d3896c..f993e6254043 100644
> > --- a/arch/x86/entry/syscall_x32.c
> > +++ b/arch/x86/entry/syscall_x32.c
> > @@ -9,15 +9,13 @@
> >  #include <asm/syscall.h>
> >
> >  #define __SYSCALL_64(nr, sym)
> > +#define __SYSCALL_COMMON(nr, sym) __SYSCALL_X32(nr, sym)
> >
> > -#define __SYSCALL_X32(nr, sym) extern long __x32_##sym(const struct pt_regs *);
> > -#define __SYSCALL_COMMON(nr, sym) extern long __x64_##sym(const struct pt_regs *);
> > +#define __SYSCALL_X32(nr, sym) extern long __x64_##sym(const struct pt_regs *);
> >  #include <asm/syscalls_64.h>
> >  #undef __SYSCALL_X32
> > -#undef __SYSCALL_COMMON
> >
> > -#define __SYSCALL_X32(nr, sym) [nr] = __x32_##sym,
> > -#define __SYSCALL_COMMON(nr, sym) [nr] = __x64_##sym,
> > +#define __SYSCALL_X32(nr, sym) [nr] = __x64_##sym,
> >
> >  asmlinkage const sys_call_ptr_t x32_sys_call_table[__NR_x32_syscall_max+1] = {
> >         /*
> > diff --git a/arch/x86/include/asm/syscall_wrapper.h b/arch/x86/include/asm/syscall_wrapper.h
> > index a84333adeef2..267fae9904ff 100644
> > --- a/arch/x86/include/asm/syscall_wrapper.h
> > +++ b/arch/x86/include/asm/syscall_wrapper.h
> > @@ -17,7 +17,7 @@ extern long __ia32_sys_ni_syscall(const struct pt_regs *regs);
> >   * __x64_sys_*()         - 64-bit native syscall
> >   * __ia32_sys_*()        - 32-bit native syscall or common compat syscall
> >   * __ia32_compat_sys_*() - 32-bit compat syscall
>
> On a 64-bit kernel, an "ia32" compat syscall is __ia32_compat_sys_*, but...
>
> > - * __x32_compat_sys_*()  - 64-bit X32 compat syscall
> > + * __x64_compat_sys_*()  - 64-bit X32 compat syscall
>
> Now an x32 compat syscall is __x64_compat?  This seems nonsensical.

Again, think of it as how the registers are mapped, not which syscall
table it belongs to.  X32 and X64 are identical in that regard.

> I'm also a bit confused as to how this is even necessary for your
> other patch.

This came out of discussion on Cristoph's patch to combine compat
execve*() into the native version:
https://lore.kernel.org/lkml/20200615141239.GA12951@xxxxxx/

The bottom line is that marking a syscall as X32-only in the syscall
table forces an __x32 prefix even if it's not a "compat" syscall.
This causes a link failure.  This is just another quirk caused by how
X32 was designed.  The solution is to make the prefix consistent for
the whole table.  The other alternative is to use __x32 for all the
common syscalls.

The second patch isn't really necessary, but it makes more sense to
not have a compat syscall with no corresponding native version.

--
Brian Gerst