vnc: threaded server depends on io-thread

Submitted by Corentin Chary on March 9, 2011, 10:41 a.m.

Details

Message ID 1299667284-15157-1-git-send-email-corentin.chary@gmail.com
State New
Headers show

Commit Message

Corentin Chary March 9, 2011, 10:41 a.m.
The threaded VNC servers messed up with QEMU fd handlers without
any kind of locking, and that can cause some nasty race conditions.

The IO-Thread provides appropriate locking primitives to avoid that.
This patch makes CONFIG_VNC_THREAD depends on CONFIG_IO_THREAD,
and add lock and unlock calls around the two faulty calls.

Thanks to Jan Kiszka for helping me solve this issue.

Cc: Jan Kiszka <jan.kiszka@web.de>
Signed-off-by: Corentin Chary <corentin.chary@gmail.com>
---
 configure           |    9 +++++++++
 ui/vnc-jobs-async.c |    4 ++++
 2 files changed, 13 insertions(+), 0 deletions(-)

Comments

Peter Lieven March 9, 2011, 10:50 a.m.
Am 09.03.2011 um 11:41 schrieb Corentin Chary:

> The threaded VNC servers messed up with QEMU fd handlers without
> any kind of locking, and that can cause some nasty race conditions.
> 
> The IO-Thread provides appropriate locking primitives to avoid that.
> This patch makes CONFIG_VNC_THREAD depends on CONFIG_IO_THREAD,
> and add lock and unlock calls around the two faulty calls.

qemu-kvm currently doesn't compile with --enable-io-thread. is there an easy fix
for this?

thanks,
peter

> 
> Thanks to Jan Kiszka for helping me solve this issue.
> 
> Cc: Jan Kiszka <jan.kiszka@web.de>
> Signed-off-by: Corentin Chary <corentin.chary@gmail.com>
> ---
> configure           |    9 +++++++++
> ui/vnc-jobs-async.c |    4 ++++
> 2 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/configure b/configure
> index 5513d3e..c8c1ac1 100755
> --- a/configure
> +++ b/configure
> @@ -2455,6 +2455,15 @@ if test \( "$cpu" = "i386" -o "$cpu" = "x86_64" \) -a \
>   roms="optionrom"
> fi
> 
> +# VNC Thread depends on IO Thread
> +if test "$vnc_thread" = "yes" -a "$io_thread" != "yes"; then
> +  echo
> +  echo "ERROR: VNC thread depends on IO thread which isn't enabled."
> +  echo "Please use --enable-io-thread if you want to enable it."
> +  echo
> +  exit 1
> +fi
> +
> 
> echo "Install prefix    $prefix"
> echo "BIOS directory    `eval echo $datadir`"
> diff --git a/ui/vnc-jobs-async.c b/ui/vnc-jobs-async.c
> index f596247..093c0d4 100644
> --- a/ui/vnc-jobs-async.c
> +++ b/ui/vnc-jobs-async.c
> @@ -260,7 +260,9 @@ static int vnc_worker_thread_loop(VncJobQueue *queue)
>         goto disconnected;
>     }
> 
> +    qemu_mutex_lock_iothread();
>     vnc_write(job->vs, vs.output.buffer, vs.output.offset);
> +    qemu_mutex_unlock_iothread();
> 
> disconnected:
>     /* Copy persistent encoding data */
> @@ -269,7 +271,9 @@ disconnected:
>     vnc_unlock_output(job->vs);
> 
>     if (flush) {
> +        qemu_mutex_lock_iothread();
>         vnc_flush(job->vs);
> +        qemu_mutex_unlock_iothread();
>     }
> 
>     vnc_lock_queue(queue);
> -- 
> 1.7.3.4
>
Corentin Chary March 9, 2011, 10:57 a.m.
>> The threaded VNC servers messed up with QEMU fd handlers without
>> any kind of locking, and that can cause some nasty race conditions.
>>
>> The IO-Thread provides appropriate locking primitives to avoid that.
>> This patch makes CONFIG_VNC_THREAD depends on CONFIG_IO_THREAD,
>> and add lock and unlock calls around the two faulty calls.
>
> qemu-kvm currently doesn't compile with --enable-io-thread. is there an easy fix
> for this?

If IO Thread is not available, I'm afraid that --disable-vnc-thread is
the only fix.
Or, you can try to define some global mutex acting like iothread
locks, but that doesn't sounds like an easy fix.
Stefan Hajnoczi March 9, 2011, 11:05 a.m.
On Wed, Mar 9, 2011 at 10:57 AM, Corentin Chary
<corentin.chary@gmail.com> wrote:
>>> The threaded VNC servers messed up with QEMU fd handlers without
>>> any kind of locking, and that can cause some nasty race conditions.
>>>
>>> The IO-Thread provides appropriate locking primitives to avoid that.
>>> This patch makes CONFIG_VNC_THREAD depends on CONFIG_IO_THREAD,
>>> and add lock and unlock calls around the two faulty calls.
>>
>> qemu-kvm currently doesn't compile with --enable-io-thread. is there an easy fix
>> for this?
>
> If IO Thread is not available, I'm afraid that --disable-vnc-thread is
> the only fix.
> Or, you can try to define some global mutex acting like iothread
> locks, but that doesn't sounds like an easy fix.

Jan or Marcelo can help here but qemu-kvm has an iothread equivalent
built in by default.  It should be possible to use that.

Stefan
Jan Kiszka March 9, 2011, 11:25 a.m.
On 2011-03-09 12:05, Stefan Hajnoczi wrote:
> On Wed, Mar 9, 2011 at 10:57 AM, Corentin Chary
> <corentin.chary@gmail.com> wrote:
>>>> The threaded VNC servers messed up with QEMU fd handlers without
>>>> any kind of locking, and that can cause some nasty race conditions.
>>>>
>>>> The IO-Thread provides appropriate locking primitives to avoid that.
>>>> This patch makes CONFIG_VNC_THREAD depends on CONFIG_IO_THREAD,
>>>> and add lock and unlock calls around the two faulty calls.
>>>
>>> qemu-kvm currently doesn't compile with --enable-io-thread. is there an easy fix
>>> for this?
>>
>> If IO Thread is not available, I'm afraid that --disable-vnc-thread is
>> the only fix.
>> Or, you can try to define some global mutex acting like iothread
>> locks, but that doesn't sounds like an easy fix.
> 
> Jan or Marcelo can help here but qemu-kvm has an iothread equivalent
> built in by default.  It should be possible to use that.

qemu_mutex_lock/unlock_iothread is properly provided in qemu-kvm even
without --enable-io-thread. So that tree could temporarily disable the
new configure check until we got rid of the special qemu-kvm bits.
Corentin's patch is against upstream, that adjustment need to be made
once the commit is merged into qemu-kvm.

Jan
Peter Lieven March 9, 2011, 11:32 a.m.
Am 09.03.2011 um 12:25 schrieb Jan Kiszka:

> On 2011-03-09 12:05, Stefan Hajnoczi wrote:
>> On Wed, Mar 9, 2011 at 10:57 AM, Corentin Chary
>> <corentin.chary@gmail.com> wrote:
>>>>> The threaded VNC servers messed up with QEMU fd handlers without
>>>>> any kind of locking, and that can cause some nasty race conditions.
>>>>> 
>>>>> The IO-Thread provides appropriate locking primitives to avoid that.
>>>>> This patch makes CONFIG_VNC_THREAD depends on CONFIG_IO_THREAD,
>>>>> and add lock and unlock calls around the two faulty calls.
>>>> 
>>>> qemu-kvm currently doesn't compile with --enable-io-thread. is there an easy fix
>>>> for this?
>>> 
>>> If IO Thread is not available, I'm afraid that --disable-vnc-thread is
>>> the only fix.
>>> Or, you can try to define some global mutex acting like iothread
>>> locks, but that doesn't sounds like an easy fix.
>> 
>> Jan or Marcelo can help here but qemu-kvm has an iothread equivalent
>> built in by default.  It should be possible to use that.
> 
> qemu_mutex_lock/unlock_iothread is properly provided in qemu-kvm even
> without --enable-io-thread. So that tree could temporarily disable the
> new configure check until we got rid of the special qemu-kvm bits.
> Corentin's patch is against upstream, that adjustment need to be made
> once the commit is merged into qemu-kvm.

do i understand you right, that i should be able to use vnc-thread together with qemu-kvm
just now if I add Corentin's patch without the io-thread dependency?

if yes, i will do and try if I can force a crash again.

br,
peter

> 
> Jan
>
Jan Kiszka March 9, 2011, 11:33 a.m.
On 2011-03-09 12:32, Peter Lieven wrote:
> 
> Am 09.03.2011 um 12:25 schrieb Jan Kiszka:
> 
>> On 2011-03-09 12:05, Stefan Hajnoczi wrote:
>>> On Wed, Mar 9, 2011 at 10:57 AM, Corentin Chary
>>> <corentin.chary@gmail.com> wrote:
>>>>>> The threaded VNC servers messed up with QEMU fd handlers without
>>>>>> any kind of locking, and that can cause some nasty race conditions.
>>>>>>
>>>>>> The IO-Thread provides appropriate locking primitives to avoid that.
>>>>>> This patch makes CONFIG_VNC_THREAD depends on CONFIG_IO_THREAD,
>>>>>> and add lock and unlock calls around the two faulty calls.
>>>>>
>>>>> qemu-kvm currently doesn't compile with --enable-io-thread. is there an easy fix
>>>>> for this?
>>>>
>>>> If IO Thread is not available, I'm afraid that --disable-vnc-thread is
>>>> the only fix.
>>>> Or, you can try to define some global mutex acting like iothread
>>>> locks, but that doesn't sounds like an easy fix.
>>>
>>> Jan or Marcelo can help here but qemu-kvm has an iothread equivalent
>>> built in by default.  It should be possible to use that.
>>
>> qemu_mutex_lock/unlock_iothread is properly provided in qemu-kvm even
>> without --enable-io-thread. So that tree could temporarily disable the
>> new configure check until we got rid of the special qemu-kvm bits.
>> Corentin's patch is against upstream, that adjustment need to be made
>> once the commit is merged into qemu-kvm.
> 
> do i understand you right, that i should be able to use vnc-thread together with qemu-kvm
> just now if I add Corentin's patch without the io-thread dependency?

Yep.

> 
> if yes, i will do and try if I can force a crash again.
> 

TIA,
Jan
Jan Kiszka March 9, 2011, 11:42 a.m.
On 2011-03-09 11:41, Corentin Chary wrote:
> The threaded VNC servers messed up with QEMU fd handlers without
> any kind of locking, and that can cause some nasty race conditions.
> 
> The IO-Thread provides appropriate locking primitives to avoid that.
> This patch makes CONFIG_VNC_THREAD depends on CONFIG_IO_THREAD,
> and add lock and unlock calls around the two faulty calls.
> 
> Thanks to Jan Kiszka for helping me solve this issue.
> 
> Cc: Jan Kiszka <jan.kiszka@web.de>
> Signed-off-by: Corentin Chary <corentin.chary@gmail.com>
> ---
>  configure           |    9 +++++++++
>  ui/vnc-jobs-async.c |    4 ++++
>  2 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/configure b/configure
> index 5513d3e..c8c1ac1 100755
> --- a/configure
> +++ b/configure
> @@ -2455,6 +2455,15 @@ if test \( "$cpu" = "i386" -o "$cpu" = "x86_64" \) -a \
>    roms="optionrom"
>  fi
>  
> +# VNC Thread depends on IO Thread
> +if test "$vnc_thread" = "yes" -a "$io_thread" != "yes"; then
> +  echo
> +  echo "ERROR: VNC thread depends on IO thread which isn't enabled."
> +  echo "Please use --enable-io-thread if you want to enable it."
> +  echo
> +  exit 1
> +fi
> +
>  
>  echo "Install prefix    $prefix"
>  echo "BIOS directory    `eval echo $datadir`"
> diff --git a/ui/vnc-jobs-async.c b/ui/vnc-jobs-async.c
> index f596247..093c0d4 100644
> --- a/ui/vnc-jobs-async.c
> +++ b/ui/vnc-jobs-async.c
> @@ -260,7 +260,9 @@ static int vnc_worker_thread_loop(VncJobQueue *queue)
>          goto disconnected;
>      }
>  
> +    qemu_mutex_lock_iothread();

Doesn't this comes with a risk of an ABBA deadlock between output_mutex
and the global lock? Here you take the global lock while holding
output_mutex, but I bet there is also the other way around when vnc
services are called from the main thread or a vcpu.

>      vnc_write(job->vs, vs.output.buffer, vs.output.offset);
> +    qemu_mutex_unlock_iothread();
>  
>  disconnected:
>      /* Copy persistent encoding data */
> @@ -269,7 +271,9 @@ disconnected:
>      vnc_unlock_output(job->vs);
>  
>      if (flush) {
> +        qemu_mutex_lock_iothread();
>          vnc_flush(job->vs);
> +        qemu_mutex_unlock_iothread();
>      }
>  
>      vnc_lock_queue(queue);

The second hunk looks safe.

Jan
Peter Lieven March 9, 2011, 12:50 p.m.
Am 09.03.2011 um 12:42 schrieb Jan Kiszka:

> On 2011-03-09 11:41, Corentin Chary wrote:
>> The threaded VNC servers messed up with QEMU fd handlers without
>> any kind of locking, and that can cause some nasty race conditions.
>> 
>> The IO-Thread provides appropriate locking primitives to avoid that.
>> This patch makes CONFIG_VNC_THREAD depends on CONFIG_IO_THREAD,
>> and add lock and unlock calls around the two faulty calls.
>> 
>> Thanks to Jan Kiszka for helping me solve this issue.
>> 
>> Cc: Jan Kiszka <jan.kiszka@web.de>
>> Signed-off-by: Corentin Chary <corentin.chary@gmail.com>
>> ---
>> configure           |    9 +++++++++
>> ui/vnc-jobs-async.c |    4 ++++
>> 2 files changed, 13 insertions(+), 0 deletions(-)
>> 
>> diff --git a/configure b/configure
>> index 5513d3e..c8c1ac1 100755
>> --- a/configure
>> +++ b/configure
>> @@ -2455,6 +2455,15 @@ if test \( "$cpu" = "i386" -o "$cpu" = "x86_64" \) -a \
>>   roms="optionrom"
>> fi
>> 
>> +# VNC Thread depends on IO Thread
>> +if test "$vnc_thread" = "yes" -a "$io_thread" != "yes"; then
>> +  echo
>> +  echo "ERROR: VNC thread depends on IO thread which isn't enabled."
>> +  echo "Please use --enable-io-thread if you want to enable it."
>> +  echo
>> +  exit 1
>> +fi
>> +
>> 
>> echo "Install prefix    $prefix"
>> echo "BIOS directory    `eval echo $datadir`"
>> diff --git a/ui/vnc-jobs-async.c b/ui/vnc-jobs-async.c
>> index f596247..093c0d4 100644
>> --- a/ui/vnc-jobs-async.c
>> +++ b/ui/vnc-jobs-async.c
>> @@ -260,7 +260,9 @@ static int vnc_worker_thread_loop(VncJobQueue *queue)
>>         goto disconnected;
>>     }
>> 
>> +    qemu_mutex_lock_iothread();
> 
> Doesn't this comes with a risk of an ABBA deadlock between output_mutex
> and the global lock? Here you take the global lock while holding
> output_mutex, but I bet there is also the other way around when vnc
> services are called from the main thread or a vcpu.

so after all i should stay with disabled vnc-thread in qemu-kvm until there is a solution?

br,
peter


> 
>>     vnc_write(job->vs, vs.output.buffer, vs.output.offset);
>> +    qemu_mutex_unlock_iothread();
>> 
>> disconnected:
>>     /* Copy persistent encoding data */
>> @@ -269,7 +271,9 @@ disconnected:
>>     vnc_unlock_output(job->vs);
>> 
>>     if (flush) {
>> +        qemu_mutex_lock_iothread();
>>         vnc_flush(job->vs);
>> +        qemu_mutex_unlock_iothread();
>>     }
>> 
>>     vnc_lock_queue(queue);
> 
> The second hunk looks safe.
> 
> Jan
> 
> -- 
> Siemens AG, Corporate Technology, CT T DE IT 1
> Corporate Competence Center Embedded Linux

Patch hide | download patch | download mbox

diff --git a/configure b/configure
index 5513d3e..c8c1ac1 100755
--- a/configure
+++ b/configure
@@ -2455,6 +2455,15 @@  if test \( "$cpu" = "i386" -o "$cpu" = "x86_64" \) -a \
   roms="optionrom"
 fi
 
+# VNC Thread depends on IO Thread
+if test "$vnc_thread" = "yes" -a "$io_thread" != "yes"; then
+  echo
+  echo "ERROR: VNC thread depends on IO thread which isn't enabled."
+  echo "Please use --enable-io-thread if you want to enable it."
+  echo
+  exit 1
+fi
+
 
 echo "Install prefix    $prefix"
 echo "BIOS directory    `eval echo $datadir`"
diff --git a/ui/vnc-jobs-async.c b/ui/vnc-jobs-async.c
index f596247..093c0d4 100644
--- a/ui/vnc-jobs-async.c
+++ b/ui/vnc-jobs-async.c
@@ -260,7 +260,9 @@  static int vnc_worker_thread_loop(VncJobQueue *queue)
         goto disconnected;
     }
 
+    qemu_mutex_lock_iothread();
     vnc_write(job->vs, vs.output.buffer, vs.output.offset);
+    qemu_mutex_unlock_iothread();
 
 disconnected:
     /* Copy persistent encoding data */
@@ -269,7 +271,9 @@  disconnected:
     vnc_unlock_output(job->vs);
 
     if (flush) {
+        qemu_mutex_lock_iothread();
         vnc_flush(job->vs);
+        qemu_mutex_unlock_iothread();
     }
 
     vnc_lock_queue(queue);