Patchwork vnc: threaded server depends on io-thread

login
register
mail settings
Submitter Corentin Chary
Date March 9, 2011, 10:41 a.m.
Message ID <1299667284-15157-1-git-send-email-corentin.chary@gmail.com>
Download mbox | patch
Permalink /patch/86098/
State New
Headers show

Comments

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

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