Message-ID: <20180406130741.GA6087@light.dominikbrodowski.net>
Date:   Fri, 6 Apr 2018 15:07:41 +0200
From:   Dominik Brodowski <linux@...inikbrodowski.net>
To:     Ingo Molnar <mingo@...nel.org>
Cc:     linux-kernel@...r.kernel.org, Al Viro <viro@...iv.linux.org.uk>,
        Andi Kleen <ak@...ux.intel.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Andy Lutomirski <luto@...nel.org>,
        Brian Gerst <brgerst@...il.com>,
        Denys Vlasenko <dvlasenk@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>, Ingo Molnar <mingo@...hat.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>, x86@...nel.org,
        Peter Zijlstra <a.p.zijlstra@...llo.nl>
Subject: Re: [PATCH 0/8] use struct pt_regs based syscall calling for x86-64

On Fri, Apr 06, 2018 at 02:34:20PM +0200, Ingo Molnar wrote:
> 
> * Dominik Brodowski <linux@...inikbrodowski.net> wrote:
> 
> > > > __sys_mprotect as prefix won't work by the way, as the double-underscore __sys_ 
> > > > variant is already used in net/* for internal syscall helpers.
> > > 
> > > Ok - then triple underscore - but overall I think it's more confusing.
> > > 
> > > Btw., what was the problem with calling the x86 ptregs wrapper sys_mprotect?
> > > 
> > > The only reason I suggested the __sys_x86_ prefix was because you originally 
> > > suggested that there's symbol name overlap, but I don't think that's the case 
> > > within the same kernel build, as the regular non-ptregs prototype:
> > 
> > Indeed, there's no symbol name overlap within the same kernel build, but
> > technically different stubs named the same. If that's fine, just drop patch
> > 8/8 (including the UML fixup) and things should be fine, with the stub and
> > the entry in the syscall table both named sys_mprotect.
> 
> Ok, I've dropped patch #8.

Thanks!

> > For IA32_EMULATION, we have __sys_ia32_mprotect as stub for the same
> > syscall, including this name as entry in syscall_32.tbl.
> > 
> > More problematic is the naming for the compat stubs for IA32_EMAULATION and
> > X32, where we have
> > 
> > 	__compat_sys_ia32_waitid
> > 	__compat_sys_x32_waitid
> > 
> > for example. We *could* rename one of those to compat_sys_waitid() and levae
> > the other as-is, but actually I prefer it now how it is.
> 
> yeah, this is more symmetric I think.
> 
> So right now we have these symbols:
> 
>  triton:~/tip> grep waitid System.map 
> 
>  ffffffff8105f1e0 t kernel_waitid               # common C function (64-bit kargs)
>  ffffffff8105f2b0 t SYSC_waitid                 # 64-bit uaddr args C function       352 bytes
>  ffffffff8105f410 T sys_waitid                  # 64-bit-ptregs -> C stub,            32 bytes
>  ffffffff8105f430 T __sys_ia32_waitid           # 32-bit-ptregs -> C stub,            32 bytes
>  ffffffff8105f450 t C_SYSC_waitid               # 32-bit uaddr args C function,      400 bytes
>  ffffffff8105f5e0 T __compat_sys_ia32_waitid    # 32-bit-ptregs -> C stub,            32 bytes
>  ffffffff8105f600 T __compat_sys_x32_waitid     # 64-bit-ptregs -> C stub,            32 bytes
> 
> BTW., what is the role of generating __sys_ia32_waitid()? It seems unused when a 
> syscall has a compat variant otherwise - like here.

Indeed it is unused -- but when compiling

	SYSCALL_DEFINE5(waitid, ...)

we don't know yet that somewhere else in the kernel there is

	COMPAT_SYSCALL_DEFINE5(waitid, ...)

So if we want to compile the __sys_ia32_ stub only iff it is used, we'd 
need to have different macros for syscalls which have compat variants and
for those which do not. Or do you have an idea how we could trick the
compiler to do the right thing?

> Naming wise the odd thumb out is sys_waitid :-/
> 
> I'd argue that we should at minimum name it __sys_waitid:

Can't do, as that namespace is already taken.

>  ffffffff8105f1e0 t kernel_waitid               # common C function (64-bit kargs)
>  ffffffff8105f2b0 t SYSC_waitid                 # 64-bit uaddr args C function
>  ffffffff8105f410 T __sys_waitid                # 64-bit-ptregs -> C stub
>  ffffffff8105f430 T __sys_ia32_waitid           # 32-bit-ptregs -> C stub
>  ffffffff8105f450 t C_SYSC_waitid               # 32-bit uaddr args C function
>  ffffffff8105f5e0 T __compat_sys_ia32_waitid    # 32-bit-ptregs -> C stub
>  ffffffff8105f600 T __compat_sys_x32_waitid     # 64-bit-ptregs -> C stub
> 
> because that makes it all organized based on the same principle:
> 
>     {__compat|_}_sys{_ia32|_x32|}_waittid
> 
> But arguably there are a whole lot more naming weirdnesses we could fix:
> 
>  - I find it somewhat confusing that that 'C' in C_SYSC stands not for a C callign 
>    convention, but for 'COMPAT' - i.e. COMPAT_SYSC would be better.
> 
>  - Another detail is that why is it called 'SYSC', if the other functions use the 
>    'sys' prefix? Wouldn't 'SYS' be more consistent?

It sounds totally reasonable to re-name those, but we should do it not only
for the x86 case, but in include/linux/syscalls.h as well.

>  - If we introduced a prefix for the regular 64-bit system call format as well,
>    we could have: x64, x32 and ia32.
> 
>  - And finally, I think the argument format modifiers should be consistently
>    prefixes - right now they are a mixture of pre- and post-fixes.
> 
> I.e. I'd generate the names like this:
> 
>     __{x64,x32,ia32}[_compat]_sys_waittid()
> 
> The fully consistent nomenclature would be someting like this:
> 
>  ffffffff8105f1e0 t            kernel_waitid    # common C function (64-bit kargs)
>  ffffffff8105f2b0 t               SYS_waitid    # 64-bit uaddr args C function
>  ffffffff8105f410 T         __x64_sys_waitid    # 64-bit-ptregs -> C stub
>  ffffffff8105f430 T        __ia32_sys_waitid    # 32-bit-ptregs -> C stub
>  ffffffff8105f450 t        COMPAT_SYS_waitid    # 32-bit uaddr args C function
>  ffffffff8105f5e0 T __ia32_compat_sys_waitid    # 32-bit-ptregs -> C stub
>  ffffffff8105f600 T  __x32_compat_sys_waitid    # 64-bit-ptregs -> C stub
> 
> Looks a lot tidier and a lot more logical, doesn't it?

Indeed. Want me to prepare a new patch 8/8 on top which does the renaming
(for x86 and for the generic case), or will you do the re-naming while
merging my patches yourself?

> Personally I'd also do a s/ia32/i32 rename:

Uh, no, please. It makes things align more beautifully, yes, but the
32-bit sub-architecture commonly is referred to as either i386 or ia32...

Thanks,
	Dominik

Powered by blists - more mailing lists