qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] Re: segmentation fault in qemu-kvm-0.14.0


From: Jan Kiszka
Subject: Re: [Qemu-devel] Re: segmentation fault in qemu-kvm-0.14.0
Date: Wed, 09 Mar 2011 11:02:37 +0100
User-agent: Mozilla/5.0 (X11; U; Linux i686 (x86_64); de; rv:1.8.1.12) Gecko/20080226 SUSE/2.0.0.12-1.1 Thunderbird/2.0.0.12 Mnenhy/0.7.5.666

On 2011-03-09 10:58, Jan Kiszka wrote:
> On 2011-03-09 10:54, Corentin Chary wrote:
>> Re-reading:
>>
>>>> So we are calling a IOHandlerRecord::fd_write handler that is NULL.
>>>> Looking at qemu_set_fd_handler2, this may happen if that function is
>>>> called for an existing io-handler entry with non-NULL write handler,
>>>> passing a NULL write and a non-NULL read handler. And all this without
>>>> the global mutex held.
>>
>> When using the vnc thread, nothing in the vnc thread will never be
>> called directly by an IO-handler. So I don't really how in would
>> trigger this.
>> And since there is a lock for accessing the output buffer (and the
>> network actually), there should not be any race condition either.
>>
>> So the real issue, is that qemu_set_fd_handler2() is called outside of
>> the main thread by those two vnc_write() and vnc_flush() calls,
>> without any kind of locking.
> 
> Yes, that's what I was referring to.
> 
>>
>>> In upstream qemu, the latter - if it exists (which is not the case in
>>> non-io-thread mode).
>>> In qemu-kvm, those locks are yet unused. Rather the code in qemu-kvm.c
>>> implements the global lock.
>>
>> So there is currently no lock for that when io-thread is disabled :/.
>> Spice also seems to project this kind of thing with
>> qemu_mutex_lock_iothread().
>>
>> Maybe qemu_mutex_lock_iothread() should also be defined when
>> CONFIG_VNC_THREAD=y ?
>>
>>> But I'm also not sure about vnc_send_framebuffer_update. Someone has to
>>> go through the threaded vnc code again, very thoroughly. It looks fragile.
>>
>> while vnc-thread is enabled vnc_send_framebuffer_update() will always
>> call vnc_write() with csock = -1 in a temporary buffer. Check
>> vnc_async_encoding_start() and vnc_async_encoding_end(), they provide
>> a kind of "sandbox" that prevent the thread to write anything the
>> main-thread will use. You can also see that as a "transaction": the
>> thread compute the update in a temporary buffer, and only send it to
>> the network (real vnc_write calls with csock correctly set) once it's
>> successfully finished.
>>
>> The is only two functions calls that break this isolation are the two
>> that I pointed out earlier.
> 
> Probably the best way is to make vnc stop fiddling with
> qemu_set_fd_handler2, specifically in threaded mode. Why does it need to
> set/reset the write handler all the time?

The other question is: Who's responsible for writing to the client
socket in threaded mode? Only the vnc thread(s), or also other qemu
threads? In the former case, just avoid setting a write handler at all.

Jan

Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]