On Fri, Apr 06, 2018 at 11:20:46AM +0200, Ingo Molnar wrote:
>
> * Dominik Brodowski <li...@dominikbrodowski.net> wrote:
>
> > On Fri, Apr 06, 2018 at 10:23:22AM +0200, Ingo Molnar wrote:
> > >
> > > * Dominik Brodowski <li...@dominikbrodowski.net> wrote:
> > >
> > > > On Thu, Apr 05, 2018 at 05:19:33PM +0200, Ingo Molnar wrote:
> > > > > Ok, this series looks mostly good to me, but AFAICS this breaks the
> > > > > UML build:
> > > > >
> > > > > make[2]: *** No rule to make target 'archheaders'. Stop.
> > > > > arch/um/Makefile:119: recipe for target 'archheaders' failed
> > > > > make[1]: *** [archheaders] Error 2
> > > > > make[1]: *** Waiting for unfinished jobs....
> > > >
> > > > Ah, that's caused by patch 8/8 which I did and do not like all that much
> > > > anyway: UML re-uses syscall_64.tbl which now has x86-specific entries
> > > > like
> > > > __sys_x86_pread64, but expects the generic syscall stub sys_pread64
> > > > referenced there. Fixup patch below; could be folded with patch 8/8. Or
> > > > patch 8/8 could simply be dropped from the series altogether...
> > >
> > > I still like the 'truth in advertising' aspect. For example if I see this
> > > in the
> > > syscall table:
> > >
> > > 10 common mprotect __sys_x86_mprotect
> > >
> > > I can immediately find the _real_ syscall entry point:
> > >
> > > ffffffff81180a10 <__sys_x86_mprotect>:
> > > ffffffff81180a10: 48 8b 57 60 mov 0x60(%rdi),%rdx
> > > ffffffff81180a14: 48 8b 77 68 mov 0x68(%rdi),%rsi
> > > ffffffff81180a18: b9 ff ff ff ff mov $0xffffffff,%ecx
> > > ffffffff81180a1d: 48 8b 7f 70 mov 0x70(%rdi),%rdi
> > > ffffffff81180a21: e8 fa fc ff ff callq ffffffff81180720
> > > <do_mprotect_pkey>
> > > ffffffff81180a26: 48 98 cltq
> > > ffffffff81180a28: c3 retq
> > > ffffffff81180a29: 0f 1f 80 00 00 00 00 nopl 0x0(%rax)
> > >
> > > If, on the other hand, I see this entry:
> > >
> > > 10 common mprotect sys_mprotect
> > >
> > > Then, as a first step, no symbol anywhere matches with this:
> > >
> > > triton:~/tip> grep sys_mprotect System.map
> > > triton:~/tip>
> > >
> > > "sys_mprotect" does not exist in any easily discoverable sense. You have
> > > to *know*
> > > to replace the sys_ prefix with __sys_x86_ to find it.
> > >
> > > Now arguably we could use a __sys_ prefix instead of the grep-barrier
> > > __sys_x86
> > > prefix - but that too would be somewhat confusing I think.
> >
> > Well, if looking at the ARCH="um" kernel, you won't find the
> > __sys_x86_mprotect
> > there in its System.map -- so we either have to disentangle um and plain
> > x86, or
> > live with some cause for confusion.
>
> I'm primarily concerned about everything making sense on x86 - UML is an
> entirely
> separate architecture with heavy tradeoffs and kludges.
Agreed.
> > __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.
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.
Thanks,
Dominik
