[Qemu-devel] [PATCH] linux-user: fix segfault deadlock

[Qemu-devel] [PATCH] linux-user: fix segfault deadlock

[Alexander Graf]

When entering the guest we take a lock to ensure that nobody else messes
with our TB chaining while we're doing it. If we get a segfault inside that
code, we manage to work on, but will not unlock the lock.

This patch forces unlocking of that lock in the segv handler. I'm not sure
this is the right approach though. Maybe we should rather make sure we don't
segfault in the code? I would greatly appreciate someone more intelligible
than me to look at this :).

Example code to trigger this is at: http://csgraf.de/tmp/conftest.c

Reported-by: Fabio Erculiani <lxnay@sabayon.org>
Signed-off-by: Alexander Graf <agraf@suse.de>
---
 qemu-lock.h |   10 ++++++++++
 user-exec.c |    4 ++++
 2 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/qemu-lock.h b/qemu-lock.h
index a72edda..e460e12 100644
--- a/qemu-lock.h
+++ b/qemu-lock.h
@@ -24,6 +24,12 @@
 #include <pthread.h>
 #define spin_lock pthread_mutex_lock
 #define spin_unlock pthread_mutex_unlock
+static inline void spin_unlock_safe(pthread_mutex_t *lock)
+{
+    /* unlocking an unlocked mutex results in undefined behavior */
+    pthread_mutex_trylock(lock);
+    pthread_mutex_unlock(lock);
+}
 #define spinlock_t pthread_mutex_t
 #define SPIN_LOCK_UNLOCKED PTHREAD_MUTEX_INITIALIZER
 
@@ -46,4 +52,8 @@ static inline void spin_unlock(spinlock_t *lock)
 {
 }
 
+static inline void spin_unlock_safe(spinlock_t *lock)
+{
+}
+
 #endif
diff --git a/user-exec.c b/user-exec.c
index abf6885..2826bd1 100644
--- a/user-exec.c
+++ b/user-exec.c
@@ -96,6 +96,10 @@ static inline int handle_cpu_signal(unsigned long pc, unsigned long address,
     qemu_printf("qemu: SIGSEGV pc=0x%08lx address=%08lx w=%d oldset=0x%08lx\n",
                 pc, address, is_write, *(unsigned long *)old_set);
 #endif
+
+    /* Maybe we're still holding the TB fiddling lock? */
+    spin_unlock_safe(&tb_lock);
+
     /* XXX: locking issue */
     if (is_write && page_unprotect(h2g(address), pc, puc)) {
         return 1;
-- 
1.6.0.2

Re: [Qemu-devel] [PATCH] linux-user: fix segfault deadlock

[Peter Maydell]

On 13 January 2012 15:46, Alexander Graf <agraf@suse.de> wrote:
> When entering the guest we take a lock to ensure that nobody else messes
> with our TB chaining while we're doing it. If we get a segfault inside that
> code, we manage to work on, but will not unlock the lock.
>
> This patch forces unlocking of that lock in the segv handler. I'm not sure
> this is the right approach though. Maybe we should rather make sure we don't
> segfault in the code? I would greatly appreciate someone more intelligible
> than me to look at this :).

A segfault while we're walking the TB chains in QEMU C code?
That's just a bug (and we know we have one there) -- we should
fix it rather than papering over it like this.

-- PMM

Re: [Qemu-devel] [PATCH] linux-user: fix segfault deadlock

[Alexander Graf]

On 13.01.2012, at 17:16, Peter Maydell wrote:

> On 13 January 2012 15:46, Alexander Graf <agraf@suse.de> wrote:
>> When entering the guest we take a lock to ensure that nobody else messes
>> with our TB chaining while we're doing it. If we get a segfault inside that
>> code, we manage to work on, but will not unlock the lock.
>> 
>> This patch forces unlocking of that lock in the segv handler. I'm not sure
>> this is the right approach though. Maybe we should rather make sure we don't
>> segfault in the code? I would greatly appreciate someone more intelligible
>> than me to look at this :).
> 
> A segfault while we're walking the TB chains in QEMU C code?
> That's just a bug (and we know we have one there) -- we should
> fix it rather than papering over it like this.

Well, we're segfaulting in this exact special case which calls setrlimit() before an mmap which fails:


agraf@wichary:/abuild/agraf/buildroot.gmime> sudo chroot .
wichary:/> QEMU_LOG=in_asm,cpu,int,out_asm QEMU_STRACE=1 ./conftest 
32595 brk(NULL) = 0x00012000
32595 uname(0x18800808) = 0
32595 mmap2(NULL,4096,PROT_READ|PROT_WRITE,MAP_PRIVATE|MAP_ANONYMOUS,-1,0) = 0x18822000
32595 access("/etc/ld.so.preload",R_OK) = -1 errno=2 (No such file or directory)
32595 open("/etc/ld.so.cache",O_RDONLY) = 4
32595 fstat64(4,0x18800430) = 0
32595 mmap2(NULL,14915,PROT_READ,MAP_PRIVATE,4,0) = 0x18823000
32595 close(4) = 0
32595 open("/lib/libc.so.6",O_RDONLY) = 4
32595 read(4,0x18800534,512) = 512
32595 fstat64(4,0x18800468) = 0
32595 mmap2(NULL,947552,PROT_EXEC|PROT_READ,MAP_PRIVATE|MAP_DENYWRITE,4,0) = 0x18827000
32595 mprotect(0x18902000,28672,PROT_NONE) = 0
32595 mmap2(0x18909000,12288,PROT_READ|PROT_WRITE,MAP_PRIVATE|MAP_DENYWRITE|MAP_FIXED,4,0xda) = 0x18909000
32595 mmap2(0x1890c000,9568,PROT_READ|PROT_WRITE,MAP_PRIVATE|MAP_ANONYMOUS|MAP_FIXED,-1,0) = 0x1890c000
32595 close(4) = 0
32595 open("/lib/libgcc_s.so.1",O_RDONLY) = 4
32595 read(4,0x1880051c,512) = 512
32595 fstat64(4,0x18800450) = 0
32595 mmap2(NULL,69908,PROT_EXEC|PROT_READ,MAP_PRIVATE|MAP_DENYWRITE,4,0) = 0x1890f000
32595 mprotect(0x18918000,28672,PROT_NONE) = 0
32595 mmap2(0x1891f000,8192,PROT_READ|PROT_WRITE,MAP_PRIVATE|MAP_DENYWRITE|MAP_FIXED,4,0x8) = 0x1891f000
32595 close(4) = 0
32595 mmap2(NULL,4096,PROT_READ|PROT_WRITE,MAP_PRIVATE|MAP_ANONYMOUS,-1,0) = 0x18921000
32595 mprotect(0x18909000,8192,PROT_READ) = 0
32595 mprotect(0x1891f000,4096,PROT_READ) = 0
32595 mprotect(0x00010000,4096,PROT_READ) = 0
32595 mprotect(0x18820000,4096,PROT_READ) = 0
32595 munmap(0x18823000,14915) = 0
32595 rt_sigaction(SIGSEGV,0x18800a00,0x18800a8c) = 0
32595 rt_sigaction(70x18800a00,0x18800a8c) = 0
32595 Unknown syscall 369
32595 ugetrlimit(2,411044936,412226752,38,411044960,2) = 0
32595 Unknown syscall 369
32595 setrlimit(2,411044936,5000000,0,411044960,2) = 0
32595 Unknown syscall 369
32595 ugetrlimit(9,411044936,412226752,38,411044960,9) = 0
32595 Unknown syscall 369
32595 setrlimit(9,411044936,5000000,0,411044960,9) = 0
32595 Unknown syscall 369
32595 ugetrlimit(3,411044936,412226752,38,411044960,3) = 0
32595 Unknown syscall 369
32595 setrlimit(3,411044936,5000000,0,411044960,3) = 0
32595 mmap2(NULL,5001216,PROT_READ|PROT_WRITE,MAP_PRIVATE|MAP_ANONYMOUS,-1,0) = 0xfffffff4

*** this is where we deadlock otherwise ***

32595 exit_group(1)


Alex

Re: [Qemu-devel] [PATCH] linux-user: fix segfault deadlock

[Fabio Erculiani]

I confirm the patch fixes the deadlock I was seeing here.

-- 
Fabio Erculiani

Re: [Qemu-devel] [PATCH] linux-user: fix segfault deadlock

[Peter Maydell]

On 13 January 2012 16:21, Alexander Graf <agraf@suse.de> wrote:
> On 13.01.2012, at 17:16, Peter Maydell wrote:
>> On 13 January 2012 15:46, Alexander Graf <agraf@suse.de> wrote:
>>> This patch forces unlocking of that lock in the segv handler. I'm not sure
>>> this is the right approach though. Maybe we should rather make sure we don't
>>> segfault in the code? I would greatly appreciate someone more intelligible
>>> than me to look at this :).
>>
>> A segfault while we're walking the TB chains in QEMU C code?
>> That's just a bug (and we know we have one there) -- we should
>> fix it rather than papering over it like this.
>
> Well, we're segfaulting in this exact special case which calls setrlimit() before an mmap which fails:

So what's actually happening here is not a problem with the failing
mmap requested by the guest, but with one we do ourselves slightly
later. The guest attempts to do a call to some code that hasn't been
translated yet (as it happens it's an attempt to call the tls function
in the ARM commpage at 0xffff0fe0, but that's not particularly
important). tb_gen_code() generates the code, and then calls
tb_link_page() to add it to our data structures. tb_link_page calls
tb_alloc_page() which calls page_find_alloc() which then needs
to allocate a level 2 page table entry in the l1_map[]. Unfortunately
(a) the mmap() it uses to do this fails because of the rlimit
(b) we don't check the mmap() return value, so we proceed on
with the bogus -1 and quickly segfault.

So "unlock and carry on regardless" is definitely wrong. We could
do the same as the system-mode (which is using g_malloc0) and
abort when the mmap() fails, although that won't help your test
program much I suspect.

For a proper fix we probably need to handle set/getrlimit for
RLIMIT_AS specially so we can apply them ourselves to the guest's
mmap/brk usage and don't get spurious allocation failures of
our private data structures...

-- PMM

Re: [Qemu-devel] [PATCH] linux-user: fix segfault deadlock

[Alexander Graf]

On 02/21/2012 05:11 PM, Peter Maydell wrote:
> On 13 January 2012 16:21, Alexander Graf<agraf@suse.de>  wrote:
>> On 13.01.2012, at 17:16, Peter Maydell wrote:
>>> On 13 January 2012 15:46, Alexander Graf<agraf@suse.de>  wrote:
>>>> This patch forces unlocking of that lock in the segv handler. I'm not sure
>>>> this is the right approach though. Maybe we should rather make sure we don't
>>>> segfault in the code? I would greatly appreciate someone more intelligible
>>>> than me to look at this :).
>>> A segfault while we're walking the TB chains in QEMU C code?
>>> That's just a bug (and we know we have one there) -- we should
>>> fix it rather than papering over it like this.
>> Well, we're segfaulting in this exact special case which calls setrlimit() before an mmap which fails:
> So what's actually happening here is not a problem with the failing
> mmap requested by the guest, but with one we do ourselves slightly
> later. The guest attempts to do a call to some code that hasn't been
> translated yet (as it happens it's an attempt to call the tls function
> in the ARM commpage at 0xffff0fe0, but that's not particularly
> important). tb_gen_code() generates the code, and then calls
> tb_link_page() to add it to our data structures. tb_link_page calls
> tb_alloc_page() which calls page_find_alloc() which then needs
> to allocate a level 2 page table entry in the l1_map[]. Unfortunately
> (a) the mmap() it uses to do this fails because of the rlimit
> (b) we don't check the mmap() return value, so we proceed on
> with the bogus -1 and quickly segfault.
>
> So "unlock and carry on regardless" is definitely wrong. We could
> do the same as the system-mode (which is using g_malloc0) and
> abort when the mmap() fails, although that won't help your test
> program much I suspect.
>
> For a proper fix we probably need to handle set/getrlimit for
> RLIMIT_AS specially so we can apply them ourselves to the guest's
> mmap/brk usage and don't get spurious allocation failures of
> our private data structures...

Or we preallocate structures we need in those critical paths. Or we 
manage an allocation failure by not linking pages.

Alex

Re: [Qemu-devel] [PATCH] linux-user: fix segfault deadlock

[Peter Maydell]

On 21 February 2012 16:19, Alexander Graf <agraf@suse.de> wrote:
> On 02/21/2012 05:11 PM, Peter Maydell wrote:
>> For a proper fix we probably need to handle set/getrlimit for
>> RLIMIT_AS specially so we can apply them ourselves to the guest's
>> mmap/brk usage and don't get spurious allocation failures of
>> our private data structures...
>
> Or we preallocate structures we need in those critical paths. Or we manage
> an allocation failure by not linking pages.

Yeah. Feel free to submit patches :-)

-- PMM