Re: [PATCH 0/3] syscalls: clean up stub naming convention

2018-04-09 Thread Linus Torvalds
On Sun, Apr 8, 2018 at 11:39 PM, Ingo Molnar  wrote:
>
> I see - so it's _not_ the same function call signature, but a wrapper with a
> sign-extended version, which is fair and useful. So on architectures where 
> this
> matters there's type conversion and active code generated.

Exactly. Some architectures have that as part of their ABI.

On Powerpc, for example, when you pass an "int" argument, the ABI
specifies that you have to sign-extend the register in the caller to
64 bits.

And the generated code actually depends on that behavior, in that
maybe it first tests the 32 bit value, but then _uses_ the full 64
bits, knowing that the caller sign-extended it properly.

This is a problem with the system call interface, since the caller
isn't a trusted entity, and user space could pass an "int" value with
the high bits set to something that _isn't_ the sign-extended thing,
so then the compiler generates unsafe code.

On x86, this never happens, since x86 doesn't have that "hidden higher
bits matter" ABI model. So that part of the wrapper will be a complete
no-op. But some other architectures depend on it.

   Linus


Re: [PATCH 0/3] syscalls: clean up stub naming convention

2018-04-09 Thread Ingo Molnar

* Dominik Brodowski  wrote:

> > New suggested naming:
> > 
> > 810f08d0 t kernel_waitid# common C function (see kernel/exit.c)
> > 
> >  __do_sys_waitid# inlined helper doing the actual work
> > # (takes original parameters as declared)
> > 
> > 810f1aa0 T   __se_sys_waitid# sign-extending C function calling 
> > inlined
> > # helper (takes parameters of type long;
> > # casts them to the declared type)
> > 
> > 810f1aa0 Tsys_waitid# alias to __se_sys_waitid() (but taking
> > # original parameters as declared), to be
> > # included in syscall table
> > 
> > Agreed?
> 
> Yes.

Ok, great. Since these re-renames look complex enough, mind re-sending the 
series 
with these changes incorporated and the 4/3 patch incorporated as well (and the 
image shrinking script left out)?

The base patches are looking good here so I plan sending these to Linus 
tomorrow-ish. (The renames don't affect functionality so they don't need
as much testing.)

Thanks,

Ingo


Re: [PATCH 0/3] syscalls: clean up stub naming convention

2018-04-09 Thread Dominik Brodowski
On Mon, Apr 09, 2018 at 09:06:11AM +0200, Ingo Molnar wrote:
> 
> * Ingo Molnar  wrote:
> 
> > 
> > * Dominik Brodowski  wrote:
> > 
> > > On Sun, Apr 08, 2018 at 10:35:50AM +0200, Ingo Molnar wrote:
> > > >   - _sys_waitid()  # ridiculous number of underscores?
> > > >   - __sys_waitid() # too generic sounding?
> > > 
> > > ... and we'd need to rename internal helpers in net/
> > > 
> > > >   - __inline_sys_waitid()  # too long?
> > > 
> > > sounds acceptable, though a bit long (especially for the compat case, 
> > > though
> > > it doesn't really matter in the case of 
> > > __inline_compat_sys_sched_rr_get_interval)
> > 
> > So as per the previous mail this is not just an inline function, but an 
> > active 
> > type conversion wrapper that sign-extends 32-bit ints to longs, which is 
> > important 
> > on some 64-bit architectures.
> > 
> > And that's a really non-obvious property IMO, and the name should probably 
> > reflect 
> > _that_ non-obvious property, not the inlining property which is really just 
> > a 
> > small detail.
> > 
> > I.e. how about:
> > 
> > __se_sys_waitid()
> > 
> > ... where 'se' stands for sign-extended, with a comment in the macro that 
> > explains 
> > the prefix? (The historical abbreviation for sign extension is 'sext', 
> > which I 
> > think wouldn't really be suitable these days.)
> 
> Ok, so I got confused there: I think it's the do_sys_waitid() intermediate 
> that
> is actually doing the sign-extension - and the inlined helper is what is in 
> the 
> syscall definition body.
> 
> So it's all still somewhat of a confusing misnomer: the 'do' named function 
> is 
> actually the sign-extension function variant - and the '_il' variant actually 
> 'does' the real work ...
> 
> I.e., old naming:
> 
> 810f08d0 t kernel_waitid  # common C function (see kernel/exit.c)
> 
>  __il_sys_waitid  # inlined helper doing the actual work
>   # (takes parameters as declared)
> 
> 810f1aa0 T   __do_sys_waitid  # C function calling inlined helper
>   # (takes parameters of type long; casts
>   #  them to the declared type)
> 
> 810f1aa0 Tsys_waitid  # alias to __do_sys_waitid() (taking
>   # parameters as declared), to be included
>   # in syscall table
> 
> 
> New suggested naming:
> 
> 810f08d0 t kernel_waitid  # common C function (see kernel/exit.c)
> 
>  __do_sys_waitid  # inlined helper doing the actual work
>   # (takes original parameters as declared)
> 
> 810f1aa0 T   __se_sys_waitid  # sign-extending C function calling inlined
>   # helper (takes parameters of type long;
>   # casts them to the declared type)
> 
> 810f1aa0 Tsys_waitid  # alias to __se_sys_waitid() (but taking
>   # original parameters as declared), to be
>   # included in syscall table
> 
> Agreed?

Yes.

Thanks,
Dominik


Re: [PATCH 0/3] syscalls: clean up stub naming convention

2018-04-09 Thread Ingo Molnar

* Ingo Molnar  wrote:

> 
> * Dominik Brodowski  wrote:
> 
> > On Sun, Apr 08, 2018 at 10:35:50AM +0200, Ingo Molnar wrote:
> > >   - _sys_waitid()  # ridiculous number of underscores?
> > >   - __sys_waitid() # too generic sounding?
> > 
> > ... and we'd need to rename internal helpers in net/
> > 
> > >   - __inline_sys_waitid()  # too long?
> > 
> > sounds acceptable, though a bit long (especially for the compat case, though
> > it doesn't really matter in the case of 
> > __inline_compat_sys_sched_rr_get_interval)
> 
> So as per the previous mail this is not just an inline function, but an 
> active 
> type conversion wrapper that sign-extends 32-bit ints to longs, which is 
> important 
> on some 64-bit architectures.
> 
> And that's a really non-obvious property IMO, and the name should probably 
> reflect 
> _that_ non-obvious property, not the inlining property which is really just a 
> small detail.
> 
> I.e. how about:
> 
>   __se_sys_waitid()
> 
> ... where 'se' stands for sign-extended, with a comment in the macro that 
> explains 
> the prefix? (The historical abbreviation for sign extension is 'sext', which 
> I 
> think wouldn't really be suitable these days.)

Ok, so I got confused there: I think it's the do_sys_waitid() intermediate that
is actually doing the sign-extension - and the inlined helper is what is in the 
syscall definition body.

So it's all still somewhat of a confusing misnomer: the 'do' named function is 
actually the sign-extension function variant - and the '_il' variant actually 
'does' the real work ...

I.e., old naming:

810f08d0 t kernel_waitid# common C function (see kernel/exit.c)

 __il_sys_waitid# inlined helper doing the actual work
# (takes parameters as declared)

810f1aa0 T   __do_sys_waitid# C function calling inlined helper
# (takes parameters of type long; casts
#  them to the declared type)

810f1aa0 Tsys_waitid# alias to __do_sys_waitid() (taking
# parameters as declared), to be included
# in syscall table


New suggested naming:

810f08d0 t kernel_waitid# common C function (see kernel/exit.c)

 __do_sys_waitid# inlined helper doing the actual work
# (takes original parameters as declared)

810f1aa0 T   __se_sys_waitid# sign-extending C function calling inlined
# helper (takes parameters of type long;
# casts them to the declared type)

810f1aa0 Tsys_waitid# alias to __se_sys_waitid() (but taking
# original parameters as declared), to be
# included in syscall table

Agreed?

Thanks,

Ingo


Re: [PATCH 0/3] syscalls: clean up stub naming convention

2018-04-09 Thread Dominik Brodowski
On Mon, Apr 09, 2018 at 08:45:11AM +0200, Ingo Molnar wrote:
> 
> * Dominik Brodowski  wrote:
> 
> > On Sun, Apr 08, 2018 at 10:35:50AM +0200, Ingo Molnar wrote:
> > >   - _sys_waitid()  # ridiculous number of underscores?
> > >   - __sys_waitid() # too generic sounding?
> > 
> > ... and we'd need to rename internal helpers in net/
> > 
> > >   - __inline_sys_waitid()  # too long?
> > 
> > sounds acceptable, though a bit long (especially for the compat case, though
> > it doesn't really matter in the case of 
> > __inline_compat_sys_sched_rr_get_interval)
> 
> So as per the previous mail this is not just an inline function, but an 
> active 
> type conversion wrapper that sign-extends 32-bit ints to longs, which is 
> important 
> on some 64-bit architectures.
> 
> And that's a really non-obvious property IMO, and the name should probably 
> reflect 
> _that_ non-obvious property, not the inlining property which is really just a 
> small detail.
> 
> I.e. how about:
> 
>   __se_sys_waitid()
> 
> ... where 'se' stands for sign-extended, with a comment in the macro that 
> explains 
> the prefix? (The historical abbreviation for sign extension is 'sext', which 
> I 
> think wouldn't really be suitable these days.)

Agreed. Could you do that when applying my patches, please?

Thanks,
Dominik


Re: [PATCH 0/3] syscalls: clean up stub naming convention

2018-04-09 Thread Dominik Brodowski
On Mon, Apr 09, 2018 at 08:49:45AM +0200, Ingo Molnar wrote:
> 
> * Dominik Brodowski  wrote:
> 
> > +emit_stub() {
> > +entry="$1"
> > +if [ "${entry}" != "${entry#__ia32_sys_}" ]; then
> > +   # We need a stub named __ia32_sys which is common to 64-bit
> > +   # except for a different pt_regs layout.
> > +   stubname=${entry#__ia32_sys_}
> > +   echo "#define __IA32_SYS_STUBx_${stubname} __IA32_SYS_STUBx"
> > +   echo "#define ASM_X86_HAS__ia32_sys_${stubname} 1"
> > +elif [ "$entry" != "${entry#__x32_compat_sys}" ]; then
> > +   # We need a stub named __x32_compat_sys_ which decodes a
> > +   # 64-bit pt_regs and then calls the real syscall function 
> > +   stubname="${entry%%/*}" # handle qualifier
> > +   stubname=${stubname#__x32_compat_sys_} # handle prefix
> > +   echo "#define __X32_COMPAT_SYS_STUBx_${stubname} __X32_COMPAT_SYS_STUBx"
> > +   echo "#define ASM_X86_HAS__x32_compat_sys_${stubname} 1"
> > +elif [ "$entry" != "${entry#__ia32_compat_sys_x86}" ]; then
> > +   # The compat entry starts with __ia32_compat_sys_x86, so it
> > +   # is a specific x86 compat syscall; no need for __ia32_sys_*()
> > +   stubname=${entry#__ia32_compat_sys_x86_}
> > +   echo "#define __IA32_SYS_STUBx_${stubname} __SYSCALL_STUBx_UNUSED"
> > +   echo "#define ASM_X86_HAS__ia32_sys_${stubname} 1"
> > +elif [ "$entry" != "${entry#__ia32_compat_sys_}" ]; then
> > +   # The compat entry starts with __ia32_compat_sys, so it is
> > +   # is a generic x86 compat syscall; no need for __ia32_sys_*()
> > +   stubname=${entry#__ia32_compat_sys_}
> > +   echo "#define __IA32_SYS_STUBx_${stubname} __SYSCALL_STUBx_UNUSED"
> > +   echo "#define ASM_X86_HAS__ia32_sys_${stubname} 1"
> > +fi;
> > +}
> 
> I only have a bikeshed painting comment, even in shell scripts please try to 
> follow the kernel comment style visually, i.e. something like:
> 
> > +if [ "${entry}" != "${entry#__ia32_sys_}" ]; then
> > +
> > +   #
> > +   # We need a stub named __ia32_sys which is common to 64-bit
> > +   # except for a different pt_regs layout.
> > +   #
> > +   stubname=${entry#__ia32_sys_}
> > +   echo "#define __IA32_SYS_STUBx_${stubname} __IA32_SYS_STUBx"
> > +   echo "#define ASM_X86_HAS__ia32_sys_${stubname} 1"
> > +
> > +elif [ "$entry" != "${entry#__x32_compat_sys}" ]; then
> > +
> > +   #
> > +   # We need a stub named __x32_compat_sys_ which decodes a
> > +   # 64-bit pt_regs and then calls the real syscall function 
> > + #
> > +   stubname="${entry%%/*}" # handle qualifier
> > +   stubname=${stubname#__x32_compat_sys_} # handle prefix
> > +   echo "#define __X32_COMPAT_SYS_STUBx_${stubname} __X32_COMPAT_SYS_STUBx"
> > +   echo "#define ASM_X86_HAS__x32_compat_sys_${stubname} 1"
> > +
> > +elif [ "$entry" != "${entry#__ia32_compat_sys_x86}" ]; then
> > +
> > + #
> > +   # The compat entry starts with __ia32_compat_sys_x86, so it
> > +   # is a specific x86 compat syscall; no need for __ia32_sys_*()
> > + #
> > +   stubname=${entry#__ia32_compat_sys_x86_}
> > +   echo "#define __IA32_SYS_STUBx_${stubname} __SYSCALL_STUBx_UNUSED"
> > +   echo "#define ASM_X86_HAS__ia32_sys_${stubname} 1"
> > +
> > +elif [ "$entry" != "${entry#__ia32_compat_sys_}" ]; then
> > +
> > + #
> > +   # The compat entry starts with __ia32_compat_sys, so it is
> > +   # is a generic x86 compat syscall; no need for __ia32_sys_*()
> > + #
> > +   stubname=${entry#__ia32_compat_sys_}
> > +   echo "#define __IA32_SYS_STUBx_${stubname} __SYSCALL_STUBx_UNUSED"
> > +   echo "#define ASM_X86_HAS__ia32_sys_${stubname} 1"
> > +
> > +fi;
> 
> ( Also note the vertical separation that helps see the overall structure a 
> bit 
>   better. )
> 
> But more fundamentally, that's an awful lot of complex scripting to save 4K 
> (unused) kernel text, not sure it's worth it.

Yes. I just wanted to see myself whether it's worthwile, and it indeed
doesn't seem so. That's exactly why I didn't sign off on it so far...

Thanks,
Dominik


Re: [PATCH 0/3] syscalls: clean up stub naming convention

2018-04-09 Thread Ingo Molnar

* Dominik Brodowski  wrote:

> +emit_stub() {
> +entry="$1"
> +if [ "${entry}" != "${entry#__ia32_sys_}" ]; then
> + # We need a stub named __ia32_sys which is common to 64-bit
> + # except for a different pt_regs layout.
> + stubname=${entry#__ia32_sys_}
> + echo "#define __IA32_SYS_STUBx_${stubname} __IA32_SYS_STUBx"
> + echo "#define ASM_X86_HAS__ia32_sys_${stubname} 1"
> +elif [ "$entry" != "${entry#__x32_compat_sys}" ]; then
> + # We need a stub named __x32_compat_sys_ which decodes a
> + # 64-bit pt_regs and then calls the real syscall function 
> + stubname="${entry%%/*}" # handle qualifier
> + stubname=${stubname#__x32_compat_sys_} # handle prefix
> + echo "#define __X32_COMPAT_SYS_STUBx_${stubname} __X32_COMPAT_SYS_STUBx"
> + echo "#define ASM_X86_HAS__x32_compat_sys_${stubname} 1"
> +elif [ "$entry" != "${entry#__ia32_compat_sys_x86}" ]; then
> + # The compat entry starts with __ia32_compat_sys_x86, so it
> + # is a specific x86 compat syscall; no need for __ia32_sys_*()
> + stubname=${entry#__ia32_compat_sys_x86_}
> + echo "#define __IA32_SYS_STUBx_${stubname} __SYSCALL_STUBx_UNUSED"
> + echo "#define ASM_X86_HAS__ia32_sys_${stubname} 1"
> +elif [ "$entry" != "${entry#__ia32_compat_sys_}" ]; then
> + # The compat entry starts with __ia32_compat_sys, so it is
> + # is a generic x86 compat syscall; no need for __ia32_sys_*()
> + stubname=${entry#__ia32_compat_sys_}
> + echo "#define __IA32_SYS_STUBx_${stubname} __SYSCALL_STUBx_UNUSED"
> + echo "#define ASM_X86_HAS__ia32_sys_${stubname} 1"
> +fi;
> +}

I only have a bikeshed painting comment, even in shell scripts please try to 
follow the kernel comment style visually, i.e. something like:

> +if [ "${entry}" != "${entry#__ia32_sys_}" ]; then
> +
> + #
> + # We need a stub named __ia32_sys which is common to 64-bit
> + # except for a different pt_regs layout.
> + #
> + stubname=${entry#__ia32_sys_}
> + echo "#define __IA32_SYS_STUBx_${stubname} __IA32_SYS_STUBx"
> + echo "#define ASM_X86_HAS__ia32_sys_${stubname} 1"
> +
> +elif [ "$entry" != "${entry#__x32_compat_sys}" ]; then
> +
> + #
> + # We need a stub named __x32_compat_sys_ which decodes a
> + # 64-bit pt_regs and then calls the real syscall function 
> + #
> + stubname="${entry%%/*}" # handle qualifier
> + stubname=${stubname#__x32_compat_sys_} # handle prefix
> + echo "#define __X32_COMPAT_SYS_STUBx_${stubname} __X32_COMPAT_SYS_STUBx"
> + echo "#define ASM_X86_HAS__x32_compat_sys_${stubname} 1"
> +
> +elif [ "$entry" != "${entry#__ia32_compat_sys_x86}" ]; then
> +
> + #
> + # The compat entry starts with __ia32_compat_sys_x86, so it
> + # is a specific x86 compat syscall; no need for __ia32_sys_*()
> + #
> + stubname=${entry#__ia32_compat_sys_x86_}
> + echo "#define __IA32_SYS_STUBx_${stubname} __SYSCALL_STUBx_UNUSED"
> + echo "#define ASM_X86_HAS__ia32_sys_${stubname} 1"
> +
> +elif [ "$entry" != "${entry#__ia32_compat_sys_}" ]; then
> +
> + #
> + # The compat entry starts with __ia32_compat_sys, so it is
> + # is a generic x86 compat syscall; no need for __ia32_sys_*()
> + #
> + stubname=${entry#__ia32_compat_sys_}
> + echo "#define __IA32_SYS_STUBx_${stubname} __SYSCALL_STUBx_UNUSED"
> + echo "#define ASM_X86_HAS__ia32_sys_${stubname} 1"
> +
> +fi;

( Also note the vertical separation that helps see the overall structure a bit 
  better. )

But more fundamentally, that's an awful lot of complex scripting to save 4K 
(unused) kernel text, not sure it's worth it.

Once we grow proper LTO support I'd guess these unused functions would go away 
naturally? None should have a __visible annotation.

Thanks,

Ingo


Re: [PATCH 0/3] syscalls: clean up stub naming convention

2018-04-09 Thread Ingo Molnar

* Dominik Brodowski  wrote:

> On Sun, Apr 08, 2018 at 10:35:50AM +0200, Ingo Molnar wrote:
> >   - _sys_waitid()  # ridiculous number of underscores?
> >   - __sys_waitid() # too generic sounding?
> 
> ... and we'd need to rename internal helpers in net/
> 
> >   - __inline_sys_waitid()  # too long?
> 
> sounds acceptable, though a bit long (especially for the compat case, though
> it doesn't really matter in the case of 
> __inline_compat_sys_sched_rr_get_interval)

So as per the previous mail this is not just an inline function, but an active 
type conversion wrapper that sign-extends 32-bit ints to longs, which is 
important 
on some 64-bit architectures.

And that's a really non-obvious property IMO, and the name should probably 
reflect 
_that_ non-obvious property, not the inlining property which is really just a 
small detail.

I.e. how about:

__se_sys_waitid()

... where 'se' stands for sign-extended, with a comment in the macro that 
explains 
the prefix? (The historical abbreviation for sign extension is 'sext', which I 
think wouldn't really be suitable these days.)

Thanks,

Ingo


Re: [PATCH 0/3] syscalls: clean up stub naming convention

2018-04-09 Thread Ingo Molnar
* Dominik Brodowski  wrote:

> > One more fundamental question: why do we have the __do_sys_waitid() and 
> > __inline_sys_waitid() distinction - aren't the function call signatures the 
> > same 
> > with no conversion done?
> > 
> > I.e. couldn't we just do a single, static __do_sys_waitid(), where the 
> > compiler 
> > would decide to what extent inlining is justified? This would allow the 
> > compiler 
> > to inline all the intermediate code into the stubs themselves.
> > 
> > Or is this a side effect of the error injection feature, which needs to add 
> > extra 
> > logic at this intermediate level? That too should be able to use the 
> > __do_sys_waitid() variant though.
> 
> Error injection is unrelated. It seems to be for three reasons, if I read
> the code (include/linux/syscalls.h) correctly:
> 
>   asmlinkage long __do_sys##name(__MAP(x,__SC_LONG,__VA_ARGS__))
> 
> 1)  This takes arguments of type long (to protect against CVE-2009-0029);
> see https://lwn.net/Articles/604287/ : "Digging into the history of
> this, it turns out that the long version ensures that 32-bit values
> are correctly sign-extended for some 64-bit kernel platforms,
> preventing a historical vulnerability."
> 
>   {
>   long ret = __in_sys##name(__MAP(x,__SC_CAST,__VA_ARGS__));
>   __MAP(x,__SC_TEST,__VA_ARGS__);

I see - so it's _not_ the same function call signature, but a wrapper with a 
sign-extended version, which is fair and useful. So on architectures where this 
matters there's type conversion and active code generated.

Thanks,

Ingo


Re: [PATCH 0/3] syscalls: clean up stub naming convention

2018-04-08 Thread Dominik Brodowski
On Sun, Apr 08, 2018 at 11:15:36AM +0200, Dominik Brodowski wrote:
> On a somewhat related note: I'll try to prepare a patch this evening which
> lets us build just the __ia32_sys and __x32_compat_sys stubs we actually
> need. We have that information already in entry/syscalls/syscall_{32,64}.tbl,
> it just needs to be extracted into another header file (in the form of
>   #define NEED_IA32_sys_xyzzz 1
> ) and then tested within the stubs. After some randconfig testing, this
> might be worthwile to add on top of the patches already in tip-asm and the
> three renaming patches currently under discussion.

So this got a bit more complicated than I though, for a -5k text size
decrease. I have my doubts whether the increased code complexity is really
worth that minor size decrease. I'll send another patch shortly, though,
which fixes up the naming of a few macros in .

--
From: Dominik Brodowski 
Date: Sun, 8 Apr 2018 21:38:54 +0200
Subject: [PATCH] syscalls/x86: only build stubs which are actually needed

A new script arch/x86/entry/syscalls/syscallstubs.sh generates
defines in 

for all __ia32_sys_ stubs which are actually needed for
IA32_EMULATION,

and

for all __x32_compat_sys_ stubs which are actually needed
for X32.

By omitting to build the remaining stubs (which are defined as
__SYSCALL_STUBx_UNUSED), there is a measurable decrease in kernel
size (-5k):

textdata  bss  dec hex filename
12892057 7094202 13570252 33556511 200081f vmlinux-orig
12886397 7087514 13570252 33544163 1ffd7e3 vmlinux

Further size cuttings could be achieved by not building stubs for
syscalls and compat_syscalls which are not referenced in the syscall
tables, such as (at least)

sys_gethostname
sys_sync_file_range2
sys_send
sys_recv
compat_sys_mbind
compat_sys_set_mempolicy
compat_sys_migrate_pages
compat_sys_sendtimedop
compat_sys_msgrcv
compat_sys_semctl
compat_sys_send
compat_sys_recv

Not-yet-signed-off-by: Dominik Brodowski 

diff --git a/arch/x86/entry/syscalls/Makefile b/arch/x86/entry/syscalls/Makefile
index 6fb9b57ed5ba..036199136513 100644
--- a/arch/x86/entry/syscalls/Makefile
+++ b/arch/x86/entry/syscalls/Makefile
@@ -11,6 +11,7 @@ syscall64 := $(srctree)/$(src)/syscall_64.tbl
 
 syshdr := $(srctree)/$(src)/syscallhdr.sh
 systbl := $(srctree)/$(src)/syscalltbl.sh
+sysstubs := $(srctree)/$(src)/syscallstubs.sh
 
 quiet_cmd_syshdr = SYSHDR  $@
   cmd_syshdr = $(CONFIG_SHELL) '$(syshdr)' '$<' '$@' \
@@ -20,6 +21,11 @@ quiet_cmd_syshdr = SYSHDR  $@
 quiet_cmd_systbl = SYSTBL  $@
   cmd_systbl = $(CONFIG_SHELL) '$(systbl)' $< $@
 
+quiet_cmd_sysstubs = SYSSTUBS  $@
+  cmd_sysstubs = $(CONFIG_SHELL) '$(sysstubs)' \
+  '$(syscall32)' '$(syscall64)' \
+   $@
+
 quiet_cmd_hypercalls = HYPERCALLS $@
   cmd_hypercalls = $(CONFIG_SHELL) '$<' $@ $(filter-out $<,$^)
 
@@ -51,6 +57,9 @@ $(out)/syscalls_32.h: $(syscall32) $(systbl)
 $(out)/syscalls_64.h: $(syscall64) $(systbl)
$(call if_changed,systbl)
 
+$(out)/syscall_stubs.h:  $(syscall32) $(syscall64) $(sysstubs)
+   $(call if_changed,sysstubs)
+
 $(out)/xen-hypercalls.h: $(srctree)/scripts/xen-hypercalls.sh
$(call if_changed,hypercalls)
 
@@ -60,6 +69,7 @@ uapisyshdr-y  += unistd_32.h unistd_64.h 
unistd_x32.h
 syshdr-y   += syscalls_32.h
 syshdr-$(CONFIG_X86_64)+= unistd_32_ia32.h unistd_64_x32.h
 syshdr-$(CONFIG_X86_64)+= syscalls_64.h
+syshdr-$(CONFIG_X86_64)+= syscall_stubs.h
 syshdr-$(CONFIG_XEN)   += xen-hypercalls.h
 
 targets+= $(uapisyshdr-y) $(syshdr-y)
diff --git a/arch/x86/entry/syscalls/syscallstubs.sh 
b/arch/x86/entry/syscalls/syscallstubs.sh
new file mode 100644
index ..4db64d4db75a
--- /dev/null
+++ b/arch/x86/entry/syscalls/syscallstubs.sh
@@ -0,0 +1,110 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+
+in32="$1"
+in64="$2"
+out="$3"
+
+emit_stub() {
+entry="$1"
+if [ "${entry}" != "${entry#__ia32_sys_}" ]; then
+   # We need a stub named __ia32_sys which is common to 64-bit
+   # except for a different pt_regs layout.
+   stubname=${entry#__ia32_sys_}
+   echo "#define __IA32_SYS_STUBx_${stubname} __IA32_SYS_STUBx"
+   echo "#define ASM_X86_HAS__ia32_sys_${stubname} 1"
+elif [ "$entry" != "${entry#__x32_compat_sys}" ]; then
+   # We need a stub named __x32_compat_sys_ which decodes a
+   # 64-bit pt_regs and then calls the real syscall function 
+   stubname="${entry%%/*}" # handle qualifier
+   stubname=${stubname#__x32_compat_sys_} # handle prefix
+   echo "#define __X32_COMPAT_SYS_STUBx_${stubname} __X32_COMPAT_SYS_STUBx"
+   echo "#define 

Re: [PATCH 0/3] syscalls: clean up stub naming convention

2018-04-08 Thread Dominik Brodowski
On Sun, Apr 08, 2018 at 10:35:50AM +0200, Ingo Molnar wrote:
>   - _sys_waitid()  # ridiculous number of underscores?
>   - __sys_waitid() # too generic sounding?

... and we'd need to rename internal helpers in net/

>   - __inline_sys_waitid()  # too long?

sounds acceptable, though a bit long (especially for the compat case, though
it doesn't really matter in the case of 
__inline_compat_sys_sched_rr_get_interval)

> One more fundamental question: why do we have the __do_sys_waitid() and 
> __inline_sys_waitid() distinction - aren't the function call signatures the 
> same 
> with no conversion done?
> 
> I.e. couldn't we just do a single, static __do_sys_waitid(), where the 
> compiler 
> would decide to what extent inlining is justified? This would allow the 
> compiler 
> to inline all the intermediate code into the stubs themselves.
> 
> Or is this a side effect of the error injection feature, which needs to add 
> extra 
> logic at this intermediate level? That too should be able to use the 
> __do_sys_waitid() variant though.

Error injection is unrelated. It seems to be for three reasons, if I read
the code (include/linux/syscalls.h) correctly:

asmlinkage long __do_sys##name(__MAP(x,__SC_LONG,__VA_ARGS__))

1)  This takes arguments of type long (to protect against CVE-2009-0029);
see https://lwn.net/Articles/604287/ : "Digging into the history of
this, it turns out that the long version ensures that 32-bit values
are correctly sign-extended for some 64-bit kernel platforms,
preventing a historical vulnerability."

{
long ret = __in_sys##name(__MAP(x,__SC_CAST,__VA_ARGS__));
__MAP(x,__SC_TEST,__VA_ARGS__);

2) We can add testing whether one of the arguments is longer than long.

__PROTECT(x, ret,__MAP(x,__SC_ARGS,__VA_ARGS__));

3) This adds asmlinkage_protect() on m68k, but seems to be a no-op on other
architectures.


While reasons 1 and 3 seem irrelevant on x86, I'd like to keep the code
close to the generic one -- and reason 2 is valid in and by itself. So I'd
recommend keeping the __inline_sys / __do_sys indirection.


> Is UML unaffected by these renames?

UML is only affected by patch 3/3, but kept happy by the patch to
entry/syscalls/syscalltbl.sh.


On a somewhat related note: I'll try to prepare a patch this evening which
lets us build just the __ia32_sys and __x32_compat_sys stubs we actually
need. We have that information already in entry/syscalls/syscall_{32,64}.tbl,
it just needs to be extracted into another header file (in the form of
#define NEED_IA32_sys_xyzzz 1
) and then tested within the stubs. After some randconfig testing, this
might be worthwile to add on top of the patches already in tip-asm and the
three renaming patches currently under discussion.


Thanks,
Dominik


Re: [PATCH 0/3] syscalls: clean up stub naming convention

2018-04-08 Thread Ingo Molnar

* Dominik Brodowski  wrote:

> In short (0x prefix removed, re-ordered):
> 
> 810f0af0 tkernel_waitid   # common (32/64) kernel helper
> 
> __in_sys_waitid   # inlined helper doing actual work
> 810f0be0 t  __do_sys_waitid   # C func calling inlined helper
> 
>  __in_compat_sys_waitid   # inlined helper doing actual work
> 810f0d80 t   __do_compat_sys_waitid   # compat C func calling inlined helper
> 
> 810f2080 T __x64_sys_waitid   # x64 64-bit-ptregs -> C stub
> 810f20b0 T__ia32_sys_waitid   # ia32 32-bit-ptregs -> C stub [unused]
> 810f2470 T __ia32_compat_sys_waitid   # ia32 32-bit-ptregs -> compat C stub
> 810f2490 T  __x32_compat_sys_waitid   # x32 64-bit-ptregs -> compat C stub

Ok, looks pretty clean and nice to me all around, and looking at the highest 
level 
syscall tables the actual calling convention and address encoding is now a 
_lot_ 
more obvious at first sight as well.

The "in" part is a tiny bit confusing because it reads like a preposition:
"are we in sys_waitid?".

But I have no better idea, other than we could perhaps use more underscores to 
signal the inline helper, instead of the 'in_' prefix:

> 810f0af0 tkernel_waitid   # common (32/64) kernel helper
> 
> _sys_waitid   # inlined helper doing actual work
> 810f0be0 t  __do_sys_waitid   # C func calling inlined helper
> 
>  _compat_sys_waitid   # inlined helper doing actual work
> 810f0d80 t   __do_compat_sys_waitid   # compat C func calling inlined helper
> 
> 810f2080 T __x64_sys_waitid   # x64 64-bit-ptregs -> C stub
> 810f20b0 T__ia32_sys_waitid   # ia32 32-bit-ptregs -> C stub [unused]
> 810f2470 T __ia32_compat_sys_waitid   # ia32 32-bit-ptregs -> compat C stub
> 810f2490 T  __x32_compat_sys_waitid   # x32 64-bit-ptregs -> compat C stub

?

There are some other variants as well, here's the list of all the options I 
could 
think of:

  - _sys_waitid()  # ridiculous number of underscores?
  - __sys_waitid() # too generic sounding?
  - __inline_sys_waitid()  # too long?
  - __il_sys_waitid()  # reminds me of the IL country code ;-)
  - __in_sys_waitid()  # easy to read as 'are we in syscall?'

None is super convinging - but maybe __inline_sys_waitid is the most natural 
one.

[ Note, whichever we pick (if we pick a new one), there no need to resend, I 
can 
  edit the patches in place if you agree. ]

One more fundamental question: why do we have the __do_sys_waitid() and 
__inline_sys_waitid() distinction - aren't the function call signatures the 
same 
with no conversion done?

I.e. couldn't we just do a single, static __do_sys_waitid(), where the compiler 
would decide to what extent inlining is justified? This would allow the 
compiler 
to inline all the intermediate code into the stubs themselves.

Or is this a side effect of the error injection feature, which needs to add 
extra 
logic at this intermediate level? That too should be able to use the 
__do_sys_waitid() variant though.

> The kbuild test robot barked at an alleged +20038 bytes kernel size regression
> for i386-tinyconfig due to the first patch of this series. That seems to be a
> false positive, as it likely doesn't take into account the change to
> scripts/bloat-o-meter. Moreover, I could not reproduce such a size regression
> on local i386 builds.

Ok, I'll ignore that.

Is UML unaffected by these renames?

Thanks,

Ingo