Date:   Fri, 6 Apr 2018 14:34:20 +0200
From:   Ingo Molnar <mingo@...nel.org>
To:     Dominik Brodowski <linux@...inikbrodowski.net>
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


* 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.

> 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.

Naming wise the odd thumb out is sys_waitid :-/

I'd argue that we should at minimum name it __sys_waitid:

 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?

 - 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?

Makes grepping easier as well, because (case-insensitive) patterns like 
'sys_waittid' would identify all the variants.

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

 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         __i32_sys_waitid    # 32-bit-ptregs -> C stub
 ffffffff8105f450 t        COMPAT_SYS_waitid    # 32-bit uaddr args C function
 ffffffff8105f5e0 T  __i32_compat_sys_waitid    # 32-bit-ptregs -> C stub
 ffffffff8105f600 T  __x32_compat_sys_waitid    # 64-bit-ptregs -> C stub

... but maybe that's too much ;-)

Thanks,

	Ingo

Powered by blists - more mailing lists