linux-user: Fix race between threads in page_unprotect()

[Qemu-devel] [PATCH 0/2] linux-user: Fix race between threads in page_unprotect()

Posted by Peter Maydell 6 years, 4 months ago

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

I'm slightly nervous about assuming that we get SEGV with SEGV_ACCERR
on a write if and only if the page permissions are set to non-writeable
(for instance https://www.mail-archive.com/tech@openbsd.org/msg40358.html
is a surprisingly recent patch to fix this for OpenBSD/x86, and I
would be unsurprised if QEMU usermode itself got the si_code wrong,
and the Boehm gc code at https://github.com/ivmai/bdwgc/blob/master/os_dep.c#L3182
is quite convoluted), but on the other hand we really ought in theory
to be able to rely on si_code, and trying to handle this some other way
gets pretty convoluted...

thanks
-- PMM

Peter Maydell (2):
  linux-user: Propagate siginfo_t through to handle_cpu_signal()
  page_unprotect(): handle calls to pages that are PAGE_WRITE

 accel/tcg/translate-all.c | 50 ++++++++++++++++++++++++++++-----------------
 accel/tcg/user-exec.c     | 52 +++++++++++++++++++++++------------------------
 2 files changed, 57 insertions(+), 45 deletions(-)

-- 
2.7.4

Re: [Qemu-devel] [PATCH 0/2] linux-user: Fix race between threads in page_unprotect()

Posted by Peter Maydell 6 years, 3 months ago

On 28 November 2017 at 14:35, Peter Maydell <peter.maydell@linaro.org> wrote:
> 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:

> Peter Maydell (2):
>   linux-user: Propagate siginfo_t through to handle_cpu_signal()
>   page_unprotect(): handle calls to pages that are PAGE_WRITE

Ping! Linux-user maintainers, any chance this could get into
a pull-request sometime soon? (I have another cleanup I'm
thinking of that will touch the same code so I'd rather this
went into master before I look at that.)

(I have a bunch of other pending linux-user patchsets which
I shan't bother to ping individually unless you want me to.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/2] linux-user: Fix race between threads in page_unprotect()

Posted by Laurent Vivier 6 years, 3 months ago

Le 15/01/2018 à 13:48, Peter Maydell a écrit :
> On 28 November 2017 at 14:35, Peter Maydell <peter.maydell@linaro.org> wrote:
>> 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:
> 
>> Peter Maydell (2):
>>   linux-user: Propagate siginfo_t through to handle_cpu_signal()
>>   page_unprotect(): handle calls to pages that are PAGE_WRITE
> 
> Ping! Linux-user maintainers, any chance this could get into
> a pull-request sometime soon? (I have another cleanup I'm
> thinking of that will touch the same code so I'd rather this
> went into master before I look at that.)
> 
> (I have a bunch of other pending linux-user patchsets which
> I shan't bother to ping individually unless you want me to.)

Riku,

if you have no time to prepare a pull request, I can.

Thanks,
Laurent

Re: [Qemu-devel] [PATCH 0/2] linux-user: Fix race between threads in page_unprotect()

Posted by Riku Voipio 6 years, 3 months ago

Hi Laurent,

On Mon, Jan 15, 2018 at 01:52:32PM +0100, Laurent Vivier wrote:
> Le 15/01/2018 à 13:48, Peter Maydell a écrit :
> > On 28 November 2017 at 14:35, Peter Maydell <peter.maydell@linaro.org> wrote:
> >> 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:
> > 
> >> Peter Maydell (2):
> >>   linux-user: Propagate siginfo_t through to handle_cpu_signal()
> >>   page_unprotect(): handle calls to pages that are PAGE_WRITE
> > 
> > Ping! Linux-user maintainers, any chance this could get into
> > a pull-request sometime soon? (I have another cleanup I'm
> > thinking of that will touch the same code so I'd rather this
> > went into master before I look at that.)
> > 
> > (I have a bunch of other pending linux-user patchsets which
> > I shan't bother to ping individually unless you want me to.)
> 
> Riku,
> 
> if you have no time to prepare a pull request, I can.

Thanks, that would be great.

Riku

Re: [Qemu-devel] [PATCH 0/2] linux-user: Fix race between threads in page_unprotect()

Posted by Laurent Vivier 6 years, 3 months ago

Le 19/01/2018 à 15:51, Riku Voipio a écrit :
> Hi Laurent,
> 
> On Mon, Jan 15, 2018 at 01:52:32PM +0100, Laurent Vivier wrote:
>> Le 15/01/2018 à 13:48, Peter Maydell a écrit :
>>> On 28 November 2017 at 14:35, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>> 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:
>>>
>>>> Peter Maydell (2):
>>>>   linux-user: Propagate siginfo_t through to handle_cpu_signal()
>>>>   page_unprotect(): handle calls to pages that are PAGE_WRITE
>>>
>>> Ping! Linux-user maintainers, any chance this could get into
>>> a pull-request sometime soon? (I have another cleanup I'm
>>> thinking of that will touch the same code so I'd rather this
>>> went into master before I look at that.)
>>>
>>> (I have a bunch of other pending linux-user patchsets which
>>> I shan't bother to ping individually unless you want me to.)
>>
>> Riku,
>>
>> if you have no time to prepare a pull request, I can.
> 
> Thanks, that would be great.

OK, I'm going to prepare it.

Thanks,
Laurent

Re: [Qemu-devel] [PATCH 0/2] linux-user: Fix race between threads in page_unprotect()

Posted by Laurent Vivier 6 years, 2 months ago

Le 19/01/2018 à 15:51, Riku Voipio a écrit :
> Hi Laurent,
> 
> On Mon, Jan 15, 2018 at 01:52:32PM +0100, Laurent Vivier wrote:
>> Le 15/01/2018 à 13:48, Peter Maydell a écrit :
>>> On 28 November 2017 at 14:35, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>> 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:
>>>
>>>> Peter Maydell (2):
>>>>   linux-user: Propagate siginfo_t through to handle_cpu_signal()
>>>>   page_unprotect(): handle calls to pages that are PAGE_WRITE
>>>
>>> Ping! Linux-user maintainers, any chance this could get into
>>> a pull-request sometime soon? (I have another cleanup I'm
>>> thinking of that will touch the same code so I'd rather this
>>> went into master before I look at that.)
>>>
>>> (I have a bunch of other pending linux-user patchsets which
>>> I shan't bother to ping individually unless you want me to.)
>>
>> Riku,
>>
>> if you have no time to prepare a pull request, I can.
> 
> Thanks, that would be great.

Riku, if you agree, I can continue to collect patches for the next
pull-request. It's easier to collect patches on the fly than scanning
the ML when we want to create a pull request.

Thanks,
Laurent

Re: [Qemu-devel] [PATCH 0/2] linux-user: Fix race between threads in page_unprotect()

Posted by Laurent Vivier 6 years, 3 months ago

Le 15/01/2018 à 13:48, Peter Maydell a écrit :
> On 28 November 2017 at 14:35, Peter Maydell <peter.maydell@linaro.org> wrote:
>> 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:
> 
>> Peter Maydell (2):
>>   linux-user: Propagate siginfo_t through to handle_cpu_signal()
>>   page_unprotect(): handle calls to pages that are PAGE_WRITE
> 
> Ping! Linux-user maintainers, any chance this could get into
> a pull-request sometime soon? (I have another cleanup I'm
> thinking of that will touch the same code so I'd rather this
> went into master before I look at that.)
> 
> (I have a bunch of other pending linux-user patchsets which
> I shan't bother to ping individually unless you want me to.)
> 
> thanks
> -- PMM
> 

I've applied this series to my linux-user branch.

I've tried to find all pending linux-user patches.

If you want to check none of yours is is missing, all patches I have
collected are in:

https://github.com/vivier/qemu/commits/linux-user-for-2.12

I'm going to run some tests before sending the pull request.

Thanks,
Laurent