| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20180406123420.kf74tgiahaugl35x@gmail.com>
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