[PATCH 0/2] X32 syscall cleanups

[PATCH 0/2] X32 syscall cleanups

[Brian Gerst]

Christoph Hellwig uncovered an issue with how we currently handle X32
syscalls.  Currently, we can only use COMPAT_SYS_DEFINEx() for X32
specific syscalls.  These changes remove that restriction and allow
native syscalls.

Brian Gerst (2):
  x86/x32: Use __x64 prefix for X32 compat syscalls
  x86/x32: Convert x32_rt_sigreturn to native syscall

 arch/x86/entry/syscall_x32.c                      |  8 +++-----
 arch/x86/entry/syscalls/syscall_64.tbl            |  2 +-
 arch/x86/include/asm/syscall_wrapper.h            | 10 +++++-----
 arch/x86/kernel/signal.c                          |  2 +-
 tools/perf/arch/x86/entry/syscalls/syscall_64.tbl |  2 +-
 5 files changed, 11 insertions(+), 13 deletions(-)


base-commit: 83cdaef93988a6bc6875623781de571b2694fe02
-- 
2.26.2

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

[Brian Gerst]

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.

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
- * __x32_compat_sys_*()  - 64-bit X32 compat syscall
+ * __x64_compat_sys_*()  - 64-bit X32 compat syscall
  *
  * The registers are decoded according to the ABI:
  * 64-bit: RDI, RSI, RDX, R10, R8, R9
@@ -165,17 +165,17 @@ extern long __ia32_sys_ni_syscall(const struct pt_regs *regs);
  * with x86_64 obviously do not need such care.
  */
 #define __X32_COMPAT_SYS_STUB0(name)					\
-	__SYS_STUB0(x32, compat_sys_##name)
+	__SYS_STUB0(x64, compat_sys_##name)
 
 #define __X32_COMPAT_SYS_STUBx(x, name, ...)				\
-	__SYS_STUBx(x32, compat_sys##name,				\
+	__SYS_STUBx(x64, compat_sys##name,				\
 		    SC_X86_64_REGS_TO_ARGS(x, __VA_ARGS__))
 
 #define __X32_COMPAT_COND_SYSCALL(name)					\
-	__COND_SYSCALL(x32, compat_sys_##name)
+	__COND_SYSCALL(x64, compat_sys_##name)
 
 #define __X32_COMPAT_SYS_NI(name)					\
-	__SYS_NI(x32, compat_sys_##name)
+	__SYS_NI(x64, compat_sys_##name)
 #else /* CONFIG_X86_X32 */
 #define __X32_COMPAT_SYS_STUB0(name)
 #define __X32_COMPAT_SYS_STUBx(x, name, ...)
-- 
2.26.2

[PATCH 2/2] x86/x32: Convert x32_rt_sigreturn to native syscall

[Brian Gerst]

x32_rt_sigreturn doesn't need to be a compat syscall because there aren't two
versions.

Signed-off-by: Brian Gerst <brgerst@gmail.com>
---
 arch/x86/entry/syscalls/syscall_64.tbl            | 2 +-
 arch/x86/kernel/signal.c                          | 2 +-
 tools/perf/arch/x86/entry/syscalls/syscall_64.tbl | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
index 78847b32e137..5fb63ac69971 100644
--- a/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/arch/x86/entry/syscalls/syscall_64.tbl
@@ -368,7 +368,7 @@
 # is defined.
 #
 512	x32	rt_sigaction		compat_sys_rt_sigaction
-513	x32	rt_sigreturn		compat_sys_x32_rt_sigreturn
+513	x32	rt_sigreturn		sys_x32_rt_sigreturn
 514	x32	ioctl			compat_sys_ioctl
 515	x32	readv			compat_sys_readv
 516	x32	writev			compat_sys_writev
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index 399f97abee02..8a3d1cd4ea70 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -856,7 +856,7 @@ void signal_fault(struct pt_regs *regs, void __user *frame, char *where)
 }
 
 #ifdef CONFIG_X86_X32_ABI
-COMPAT_SYSCALL_DEFINE0(x32_rt_sigreturn)
+SYSCALL_DEFINE0(x32_rt_sigreturn)
 {
 	struct pt_regs *regs = current_pt_regs();
 	struct rt_sigframe_x32 __user *frame;
diff --git a/tools/perf/arch/x86/entry/syscalls/syscall_64.tbl b/tools/perf/arch/x86/entry/syscalls/syscall_64.tbl
index 37b844f839bc..36a3c8a913da 100644
--- a/tools/perf/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/tools/perf/arch/x86/entry/syscalls/syscall_64.tbl
@@ -367,7 +367,7 @@
 # is defined.
 #
 512	x32	rt_sigaction		compat_sys_rt_sigaction
-513	x32	rt_sigreturn		compat_sys_x32_rt_sigreturn
+513	x32	rt_sigreturn		sys_x32_rt_sigreturn
 514	x32	ioctl			compat_sys_ioctl
 515	x32	readv			compat_sys_readv
 516	x32	writev			compat_sys_writev
-- 
2.26.2

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

[Andy Lutomirski]

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.
I'm also a bit confused as to how this is even necessary for your
other patch.

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@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

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

[hpa]

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.

Re: [PATCH 0/2] X32 syscall cleanups

[Christoph Hellwig]

On Tue, Jun 16, 2020 at 10:23:13AM -0400, Brian Gerst wrote:
> Christoph Hellwig uncovered an issue with how we currently handle X32
> syscalls.  Currently, we can only use COMPAT_SYS_DEFINEx() for X32
> specific syscalls.  These changes remove that restriction and allow
> native syscalls.

Did this go anywhere?

Re: [PATCH 0/2] X32 syscall cleanups

[Brian Gerst]

On Tue, Jul 14, 2020 at 2:40 AM Christoph Hellwig <hch@lst.de> wrote:
>
> On Tue, Jun 16, 2020 at 10:23:13AM -0400, Brian Gerst wrote:
> > Christoph Hellwig uncovered an issue with how we currently handle X32
> > syscalls.  Currently, we can only use COMPAT_SYS_DEFINEx() for X32
> > specific syscalls.  These changes remove that restriction and allow
> > native syscalls.
>
> Did this go anywhere?

This approach wasn't well received, so I'd just go with this as the
simplest solution:
https://lore.kernel.org/lkml/CAK8P3a17h782gO65qJ9Mmz0EuiTSKQPEyr_=nvqOtnmQZuh9Kw@mail.gmail.com/

--
Brian Gerst

Re: [PATCH 0/2] X32 syscall cleanups

[Andy Lutomirski]

On Tue, Jul 14, 2020 at 10:03 AM Brian Gerst <brgerst@gmail.com> wrote:>
> On Tue, Jul 14, 2020 at 2:40 AM Christoph Hellwig <hch@lst.de> wrote:
> >
> > On Tue, Jun 16, 2020 at 10:23:13AM -0400, Brian Gerst wrote:
> > > Christoph Hellwig uncovered an issue with how we currently handle X32
> > > syscalls.  Currently, we can only use COMPAT_SYS_DEFINEx() for X32
> > > specific syscalls.  These changes remove that restriction and allow
> > > native syscalls.
> >
> > Did this go anywhere?
>
> This approach wasn't well received, so I'd just go with this as the
> simplest solution:
> https://lore.kernel.org/lkml/CAK8P3a17h782gO65qJ9Mmz0EuiTSKQPEyr_=nvqOtnmQZuh9Kw@mail.gmail.com/
>

I'm okay with either approach, although I think the original approach
is nicer than the simplified #define approach.

In my mind, the __x64_omg_so_many_underscores prefixes really mean
"don't think too hard about these -- we just decided to make extra
long names", so whatever.  We can clean it up more some day.

> --
> Brian Gerst