[PATCH v3 07/13] loader: Don't clobber existing memory mappings when reserving addresses.

Thu Jan 27 08:52:42 CST 2022

On 1/27/22 16:37, Alexandre Julliard wrote:
> Jinoh Kang <jinoh.kang.kr at gmail.com> writes:
> 
>> - loader: Use long instead of int for syscall return type in i386 code.
>>
>>   I was planning to add more syscalls, and found that i386 code used "int" for
>>   syscall returns. My assumption was that it was a style issue (presumably
>>   32-bit-area Wine's leftover) that I was expected to address _before_ I could
>>   add more inline asm blocks.
>>
>>   [The reason why I found "int" confusing was that I usually read "long" as
>>    "machine register-width integer," and "int" as "integer that either only
>>    needs to hold small values (e.g. exit code) _or_ has to be 32-bit for some
>>    reason."  Granted int = long on ILP32, but someone looking at the
>>    code might not be able to immediately verify that the type is correct unless
>>    they also consider that "int $0x80" is for the i386 ABI (or that the code region
>>    is guarded by ifdef __i386__), and i386 is a 32-bit architecture.
>>    I supposed that would be extra cognitive load for someone reviewing/auditing
>>    the code.]
> 
> "long" doesn't mean pointer size in Win32, and since it differs between
> Win32 and Unix it's confusing, so we try to avoid it as much as
> possible. Here it's obviously safe, but that still makes it more
> confusing than "int", not less.

Thanks for the comment.  It sounds reasonable.  Perhaps if I were going for
readability, I should have used "uintptr_t" instead.

I'll switch to int for the next iteration.

> 
>> - loader: Remove GCC <=4.x EBX register spilling workaround for i386.
>>
>>   Similar to above, but debugging related.  Again I was adding asm blocks
>>   around, so I felt it obliged to fix it before adding more code.
> 
> I'm not convinced that this is safe, did you test on a really old gcc?

Yes, I did.  See:

  https://godbolt.org/z/nq1T1rr93  (gcc -m32 -O2 -fno-PIC)

versus:

  https://godbolt.org/z/8ebsK7Mzx  (gcc -m32 -O2 -fPIC)

Note that using EBX works flawlessly if PIC is disabled (which is the case for the preloader).

Furthermore I have verified that the patch works on CentOS 7 with GCC 4.8.5:

$ gcc --version
gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-44)
Copyright (C) 2015 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

$ WINEPRELOADREMAPVDSO=always WINEPRELOADREMAPSTACK=always strace -e trace=mremap ./wine winecfg
--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=23970, si_uid=1000, si_status=0, si_utime=0, si_stime=0} ---
--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=23971, si_uid=1000, si_status=0, si_utime=0, si_stime=0} ---
--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=23972, si_uid=1000, si_status=0, si_utime=0, si_stime=0} ---
--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=23973, si_uid=1000, si_status=0, si_utime=0, si_stime=0} ---
strace: [ Process PID=23969 runs in 32 bit mode. ]
mremap(0xf127d000, 12288, 12288, MREMAP_MAYMOVE|MREMAP_FIXED, 0xf1278000) = 0xf1278000
mremap(0xf1280000, 4096, 4096, MREMAP_MAYMOVE|MREMAP_FIXED, 0xf127b000) = 0xf127b000
--- SIGIO {si_signo=SIGIO, si_code=SI_USER, si_pid=23969, si_uid=1000} ---
--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=23974, si_uid=1000, si_status=0, si_utime=0, si_stime=0} ---
--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=23976, si_uid=1000, si_status=0, si_utime=0, si_stime=0} ---
--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=23978, si_uid=1000, si_status=0, si_utime=0, si_stime=0} ---
--- SIGUSR1 {si_signo=SIGUSR1, si_code=SI_TKILL, si_pid=23975, si_uid=1000} ---
--- SIGUSR1 {si_signo=SIGUSR1, si_code=SI_TKILL, si_pid=23975, si_uid=1000} ---
--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=23980, si_uid=1000, si_status=0, si_utime=0, si_stime=0} ---
0044:err:ole:start_rpcss Failed to open service manager
003c:fixme:imm:ImeSetActiveContext (0x25ae88, 1): stub
003c:fixme:imm:ImmReleaseContext (00010054, 0025AE88): stub
0044:fixme:imm:ImeSetActiveContext (0x257eb0, 0): stub
0044:fixme:imm:ImmReleaseContext (00010020, 00257EB0): stub

> 
>> Meanwhile, following are patches that I deem necessary and not gratuitous:
>>
>> - loader: Refactor argv/envp/auxv management.
>>
>>   This was necessary because I had to pass around the pointers to
>>   stack objects a lot.  Examples include getenv() for letting
>>   user control remapping behaviour via env vars, and stack relocation
>>   (which requires shifting all the argv/envp/auxv pointers).
>>   If the pointer variables were not in one place, the latter would
>>   make the code a lot hairy (by passing around all the scattered
>>   stack pointers) and unmaintainable.
> 
> Well, letting user control it through env vars is exactly what I mean
> with "unneeded infrastructure".

It was intended to avoid regression by gradually adopting the new behaviour.

My plan was to:

1. Hide the new code path behind a flag.
2. Make it default on Wine-Staging  (https://bugs.winehq.org/show_bug.cgi?id=52313).
3. Instruct users experiencing performance problem (intermittent or persistent)
   to enable the new behaviour and see if it fixes the problem.
4. If enough users have tested the new feature, make it non-configurable.

If you think this process is unnecessary, the environment variables are of course nonsense.

(I've got no replies on the ticket there, and I guessed it might have been better off on the list after all...)

> No user is going to want to understand
> or tweak these things.

Yes. It is intended only for testers and tech/community support, not end-users.

> 
>> - loader: Refactor number parsing to own function.
>>
>>   Number parsing was only for WINEPRELOADRESERVE.  Now I need it for
>>   parsing /proc/self/maps as well.
> 
> You shouldn't need to parse /proc/self/maps at all.
> 

Honestly the part I spent the most time on this patchset was to try to avoid having to parse /proc/self/maps entirely.

The problem is that it's impossible to reliably identity the exact range of Linux vDSO/vvar mapping without reading /proc/self/maps.

1. vDSO hard-codes vvar's offset relative to vDSO. Therefore, remapping vDSO requires vvar to be *also* remapped as well.
   However, vvar's size and its location relative to vDSO is *not* guaranteed by ABI, and has changed all the time.

   - x86: [vvar] orginally resided at a fixed address 0xffffffffff5ff000 (64-bit) [1], but was later changed so that it precedes [vdso] [2].
     There, sym_vvar_start is a negative value [3]. text_start is the base address of vDSO, and addr becomes the address of vvar.

   - AArch32: [vvar] is a single page and precedes [vdso] [4].

   - AArch64: [vvar] is two pages long and precedes [vdso] [5].  Before v5.9, [vvar] was a single page [6].

2. It's very difficult to deduce vDSO and vvar's size and offset relative to each other.
   Since vvar's symbol does not exist in vDSO's symtab, determining the layout would require parsing vDSO's code.

Also note that CRIU (Checkpoint Restore In Userspace) has maps parsing code just for relocating vDSO [7].

[1] https://lwn.net/Articles/615809/
[2] https://elixir.bootlin.com/linux/v5.16.3/source/arch/x86/entry/vdso/vma.c#L246
[3] https://elixir.bootlin.com/linux/v5.16.3/source/arch/x86/include/asm/vdso.h#L21
[4] https://elixir.bootlin.com/linux/v5.16.3/source/arch/arm/kernel/vdso.c#L236
[5] https://elixir.bootlin.com/linux/v5.16.3/source/arch/arm64/kernel/vdso.c#L214
[6] https://elixir.bootlin.com/linux/v5.8/source/arch/arm64/kernel/vdso.c#L161
[7] https://github.com/checkpoint-restore/criu/blob/a315774e11b4da1eb36446ae996eac1695a129a6/criu/vdso.c

-- 
Sincerely,
Jinoh Kang