Re: [Qemu-devel] [PULL 12/13] page_unprotect(): handle calls to pages that are PAGE_WRITE

[Laurent Vivier]

Le 22/03/2018 à 02:52, Laurent Vivier a écrit :
> Le 23/01/2018 à 15:48, Laurent Vivier a écrit :
>> From: Peter Maydell <address@hidden>
>>
>> If multiple guest threads in user-mode emulation write to a
>> page which QEMU has marked read-only because of cached TCG
>> translations, the threads can race in page_unprotect:
>>
>>  * threads A & B both try to do a write to a page with code in it at
>>    the same time (ie which we've made non-writeable, so SEGV)
>>  * they race into the signal handler with this faulting address
>>  * thread A happens to get to page_unprotect() first and takes the
>>    mmap lock, so thread B sits waiting for it to be done
>>  * A then finds the page, marks it PAGE_WRITE and mprotect()s it writable
>>  * A can then continue OK (returns from signal handler to retry the
>>    memory access)
>>  * ...but when B gets the mmap lock it finds that the page is already
>>    PAGE_WRITE, and so it exits page_unprotect() via the "not due to
>>    protected translation" code path, and wrongly delivers the signal
>>    to the guest rather than just retrying the access
>>
>> In particular, this meant that trying to run 'javac' in user-mode
>> emulation would fail with a spurious guest SIGSEGV.
>>
>> Handle this by making page_unprotect() assume that a call for a page
>> which is already PAGE_WRITE is due to a race of this sort and return
>> a "fault handled" indication.
>>
>> Since this would cause an infinite loop if we ever called
>> page_unprotect() for some other kind of fault than "write failed due
>> to bad access permissions", tighten the condition in
>> handle_cpu_signal() to check the signal number and si_code, and add a
>> comment so that if somebody does ever find themselves debugging an
>> infinite loop of faults they have some clue about why.
>>
>> (The trick for identifying the correct setting for
>> current_tb_invalidated for thread B (needed to handle the precise-SMC
>> case) is due to Richard Henderson.  Paolo Bonzini suggested just
>> relying on si_code rather than trying anything more complicated.)
>>
>> Signed-off-by: Peter Maydell <address@hidden>
>> Message-Id: <address@hidden>
>> Signed-off-by: Laurent Vivier <address@hidden>
>> ---
>>  accel/tcg/translate-all.c | 50 
>> +++++++++++++++++++++++++++++------------------
>>  accel/tcg/user-exec.c     | 13 +++++++++++-
>>  2 files changed, 43 insertions(+), 20 deletions(-)
>>
> 
> It seems this patch breaks something in linux-user mode emulation for
> m68k (32bit BE) on ppc (32bit BE).
> 
> What I have:
> 
>   ~/chroot$ sudo QEMU_CPU=m68040 chroot m68k/sid/
>   I have no address@hidden:/# ls
>   bin   debootstrap  etc       lib   qemu-m68k  run   sys  usr
>   boot  dev      home  proc  root       sbin  tmp  var
>   qemu: uncaught target signal 11 (Segmentation fault) - core dumped
>   ~/chroot$
> 
> It seems "bash" crashes on "ls" exit.
> 
> My chroot has been installed with:
> 
>   ARCH=m68k
>   TARGET=sid
>   CHROOT=$HOME/chroot/m68k/sid/
>   REPOT=http://cdn-fastly.deb.debian.org/debian-ports/
>   debootstrap --arch=$ARCH --foreign --variant=minbase \
>               --no-check-gpg $TARGET $CHROOT $REPO
> 
> I didn't investigate more.

It goes wrong in this part:

+     */
+    if (is_write && info->si_signo == SIGSEGV && info->si_code ==
SEGV_ACCERR &&
+        h2g_valid(address)) {

Because, on ppc, si_code is SEGV_MAPERR and not SEGV_ACCERR
(on x86_64, si_code is SEGV_ACCERR as expected)

Any idea?

Thanks,
Laurent