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

hpa
2020-06-23T20:50:39Z

On June 16, 2020 10:17:29 AM PDT, Brian Gerst <brgerst@gmail.com> wrote:
>On Tue, Jun 16, 2020 at 12:49 PM Andy Lutomirski <luto@kernel.org>
>wrote:
>>
>> On Tue, Jun 16, 2020 at 7:23 AM Brian Gerst <brgerst@gmail.com>
>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@gmail.com>
>> > ---
>> > 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@lst.de/
>
>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

Please don't use "x64" to mean anything other than x86-64, as some, ahem, other OSes use those as synonyms.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.