Re: [PATCH 0/3] syscalls: clean up stub naming convention
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
* 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
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
* 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
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
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
* 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
* 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
* 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
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 ASM_X86_HAS__x32_compat_sys_${stubname} 1"
+elif [ "$entry"
Re: [PATCH 0/3] syscalls: clean up stub naming convention
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
* 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
