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@xxxxxx>
> Signed-off-by: Corentin Chary <corentin.chary@xxxxxxxxx>
> ---
> 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
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
