Re: [PATCH] x86/entry: Fix SYS_NI() build failure

[Brian Gerst]

On Mon, Mar 23, 2020 at 4:11 AM Ingo Molnar <mingo@xxxxxxxxxx> wrote:
>
>
> * Brian Gerst <brgerst@xxxxxxxxx> wrote:
>
> > Pull the common code out from the SYS_NI macros into a new __SYS_NI macro.
> > Also conditionalize the X64 version in preparation for enabling syscall
> > wrappers on 32-bit native kernels.
> >
> > Signed-off-by: Brian Gerst <brgerst@xxxxxxxxx>
> > Reviewed-by: Dominik Brodowski <linux@xxxxxxxxxxxxxxxxxxxx>
> > Reviewed-by: Andy Lutomirski <luto@xxxxxxxxxx>
>
> >  #define COMPAT_SYS_NI(name)                                          \
> > -     SYSCALL_ALIAS(__ia32_compat_sys_##name, sys_ni_posix_timers);   \
> > -     SYSCALL_ALIAS(__x32_compat_sys_##name, sys_ni_posix_timers)
> > +     __IA32_COMPAT_SYS_NI(name)                                      \
> > +     __X32_COMPAT_SYS_NI(name)
> >
> >  #endif /* CONFIG_COMPAT */
> >
> > @@ -231,9 +245,9 @@ struct pt_regs;
> >       __X64_COND_SYSCALL(name)                                        \
> >       __IA32_COND_SYSCALL(name)
> >
> > -#ifndef SYS_NI
> > -#define SYS_NI(name) SYSCALL_ALIAS(__x64_sys_##name, sys_ni_posix_timers);
> > -#endif
> > +#define SYS_NI(name)                                                 \
> > +     __X64_SYS_NI(name)                                              \
> > +     __IA32_SYS_NI(name)
>
> This breaks the x86-64 build on !CONFIG_POSIX_TIMERS (and probably also
> with some x32 build variants), because of a missing ';' between
> __X64_SYS_NI() and __IA32_SYS_NI().
>
> I suspect testing didn't catch this because SYS_NI() is rarely used in
> our x86-64 Kconfig space.
>
> Very lightly tested fix attached.
>
> I didn't see the COND_SYSCALL_COMPAT() build failure, but seems to have a
> similar bug.
>
> Thanks,
>
>         Ingo
>
> Signed-off-by: Ingo Molnar <mingo@xxxxxxxxxx>
>
> ---
>  arch/x86/include/asm/syscall_wrapper.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/include/asm/syscall_wrapper.h b/arch/x86/include/asm/syscall_wrapper.h
> index e10efa1454bc..8929419b9783 100644
> --- a/arch/x86/include/asm/syscall_wrapper.h
> +++ b/arch/x86/include/asm/syscall_wrapper.h
> @@ -214,12 +214,12 @@ extern long __ia32_sys_ni_syscall(const struct pt_regs *regs);
>   * COND_SYSCALL_COMPAT in kernel/sys_ni.c and COMPAT_SYS_NI in
>   * kernel/time/posix-stubs.c to cover this case as well.
>   */
> -#define COND_SYSCALL_COMPAT(name)                                      \
> -       __IA32_COMPAT_COND_SYSCALL(name)                                \
> +#define COND_SYSCALL_COMPAT(name)                                      \
> +       __IA32_COMPAT_COND_SYSCALL(name);                               \
>         __X32_COMPAT_COND_SYSCALL(name)
>
>  #define COMPAT_SYS_NI(name)                                            \
> -       __IA32_COMPAT_SYS_NI(name)                                      \
> +       __IA32_COMPAT_SYS_NI(name);                                     \
>         __X32_COMPAT_SYS_NI(name)
>
>  #endif /* CONFIG_COMPAT */
> @@ -257,7 +257,7 @@ extern long __ia32_sys_ni_syscall(const struct pt_regs *regs);
>         __IA32_COND_SYSCALL(name)
>
>  #define SYS_NI(name)                                                   \
> -       __X64_SYS_NI(name)                       6e4847640c6aebcaa2d9b3686cecc91b41f09269                       \
> +       __X64_SYS_NI(name);                                             \
>         __IA32_SYS_NI(name)
>
>

A simpler fix would be to add the semicolon to the end of the __SYS_NI
macro.  COND_SYSCALL_COMPAT isn't a problem because it expands to a
function instead of an asm statement.

That said, I think __SYS_NI should be changed to mirror
__COND_SYSCALL, so that the function prototypes match (see commit
6e484764)

--
Brian Gerst