Re: [PATCH 1/5] x86: Refactor syscall_wrapper.h

[Brian Gerst <brgerst@gmail.com>]

On Fri, Feb 21, 2020 at 2:07 AM Dominik Brodowski
<linux@dominikbrodowski.net> wrote:
>
> > + * Instead of the generic __SYSCALL_DEFINEx() definition, this macro takes
>
> If you move the description to the top (the reason for that is
> understandable), you can't talk about "this macro" any more --
> as "this macro" is "__SYSCALL_DEFINEx()" which is defined far further
> below.
>
> > + * struct pt_regs *regs as the only argument of the syscall stub named
> > + * __x64_sys_*(). It decodes just the registers it needs and passes them on to
> > + * the __se_sys_*() wrapper performing sign extension and then to the
> > + * __do_sys_*() function doing the actual job. These wrappers and functions
> > + * are inlined (at least in very most cases), meaning that the assembly looks
> > + * as follows (slightly re-ordered for better readability):
> > + *
> > + * <__x64_sys_recv>:         <-- syscall with 4 parameters
> > + *   callq   <__fentry__>
> > + *
> > + *   mov     0x70(%rdi),%rdi <-- decode regs->di
> > + *   mov     0x68(%rdi),%rsi <-- decode regs->si
> > + *   mov     0x60(%rdi),%rdx <-- decode regs->dx
> > + *   mov     0x38(%rdi),%rcx <-- decode regs->r10
> > + *
> > + *   xor     %r9d,%r9d       <-- clear %r9
> > + *   xor     %r8d,%r8d       <-- clear %r8
> > + *
> > + *   callq   __sys_recvfrom  <-- do the actual work in __sys_recvfrom()
> > + *                               which takes 6 arguments
> > + *
> > + *   cltq                    <-- extend return value to 64-bit
> > + *   retq                    <-- return
> > + *
> > + * This approach avoids leaking random user-provided register content down
> > + * the call chain.
>
> ... maybe add the rationale for x86-32 here (in patch 3/5)?
>
> > + * If IA32_EMULATION is enabled, this macro generates an additional wrapper
> > + * named __ia32_sys_*() which decodes the struct pt_regs *regs according
> > + * to the i386 calling convention (bx, cx, dx, si, di, bp).
>
> ... and this likely needs an update in patch 3/5 as well

I will update that comment section.  The code for 32-bit will follow
the same pattern, just with different register names.

> > -#define __IA32_COMPAT_SYS_STUBx(x, name, ...)                                \
> > -     asmlinkage long __ia32_compat_sys##name(const struct pt_regs *regs);\
> > -     ALLOW_ERROR_INJECTION(__ia32_compat_sys##name, ERRNO);          \
> > -     asmlinkage long __ia32_compat_sys##name(const struct pt_regs *regs)\
> > +#define __SYS_STUB0(abi, name)                                               \
> > +     asmlinkage long __##abi##_##name(const struct pt_regs *regs);   \
> > +     ALLOW_ERROR_INJECTION(__##abi##_##name, ERRNO);                 \
> > +     asmlinkage long __##abi##_##name(const struct pt_regs *regs)    \
> >       {                                                               \
> > -             return __se_compat_sys##name(SC_IA32_REGS_TO_ARGS(x,__VA_ARGS__));\
> > +             return __do_##name();                                   \
> >       }
>
> Instead of __se_compat_sys##name, now __do_comapt_sys##name is called. This
> should be equivalent for zero-argument syscalls, but maybe make that change
> explicit.

The __se_* layer (sign-extension) is unnecessary when there are no
arguments to extend.  But I can make that a separate patch if that
makes it clearer.

> In general terms, I am not sure that the new code is more readable (but I
> may be biased) as the conversion to the proper calling convention for the
> ABI has to be done by the callee and is not done in the caller. Also, there
> are now three levels of macros instead of two.

I added the __STUBx layer to avoid 4 copies of the same boilerplate
code.  The middle layer is needed because you can't have #ifdefs
inside a macro.

>
>
> > +#ifdef CONFIG_X86_64
> > +#define __X64_SYS_STUBx(x, name, ...)                                        \
> > +     __SYS_STUBx(x64, name, SC_X86_64_REGS_TO_ARGS(x, __VA_ARGS__))
>
> s/name/sys_name/g please -- this isn't the name of the syscall any more, but
> appended by sys (applies to a number of macros)
>
> > - * named __ia32_sys_*()
> > + * As the generic SYSCALL_DEFINE0() macro does not decode any parameters for
> > + * obvious reasons, and passing struct pt_regs *regs to it in %rdi does not
> > + * hurt, we only need to re-define it here to keep the naming congruent to
> > + * SYSCALL_DEFINEx() -- which is essential for the COND_SYSCALL() and SYS_NI()
> > + * macros to work correctly.
> >   */
> > +#define SYSCALL_DEFINE0(name)                                        \
> > +     SYSCALL_METADATA(_##name, 0);                           \
> > +     static inline long __do_sys_##name(void);               \
> > +     __X64_SYS_STUB0(sys_##name)                             \
> > +     __IA32_SYS_STUB0(sys_##name)                            \
> > +     static inline long __do_sys_##name(void)
> >
> > -#define SYSCALL_DEFINE0(sname)                                               \
> > -     SYSCALL_METADATA(_##sname, 0);                                  \
> > -     asmlinkage long __x64_sys_##sname(const struct pt_regs *__unused);\
> > -     ALLOW_ERROR_INJECTION(__x64_sys_##sname, ERRNO);                \
> > -     SYSCALL_ALIAS(__ia32_sys_##sname, __x64_sys_##sname);           \
> > -     asmlinkage long __x64_sys_##sname(const struct pt_regs *__unused)
>
> OK, this is a bit more complex change -- you don't use SYSCALL_ALIAS any
> more, but define two asmlinakge functions which then call __do_sys_##name().
> Maybe needs an explanation in the changelog...

IIRC this was because in the 32-bit native case the __x64_* variant
wasn't present to alias to.  I'll see if I can come up with a
different solution.

> > +#define __IA32_COMPAT_SYS_STUBx(x, name, ...)                                \
> > +     __SYS_STUBx(ia32, name, SC_IA32_REGS_TO_ARGS(x, __VA_ARGS__))
>
> s/name/compat_sys_name/g/ -- please make it a bit more explicit what "name"
> we are talking about here,
>
> > +#define __IA32_COMPAT_SYS_STUB0(name)                                        \
> > +     __SYS_STUB0(ia32, name)
>
> and here,
>
> > +#define __IA32_COMPAT_COND_SYSCALL(name)                             \
> > +     __COND_SYSCALL(ia32, name)
>
> and here,
>
> > +#define __IA32_COMPAT_SYS_NI(name)                                   \
> > +     SYSCALL_ALIAS(__ia32_compat_sys_##name, sys_ni_posix_timers)
>
> ... but not here, as here "name" is actually "name".
>
> Thanks,
>         Dominik

--
Brian Gerst