From patchwork Fri Sep 2 13:39:03 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Gerd Hoffmann X-Patchwork-Id: 113140 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [140.186.70.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 06D39B6F83 for ; Fri, 2 Sep 2011 23:39:22 +1000 (EST) Received: from localhost ([::1]:58399 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QzTxm-0002be-Sa for incoming@patchwork.ozlabs.org; Fri, 02 Sep 2011 09:39:18 -0400 Received: from eggs.gnu.org ([140.186.70.92]:51531) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QzTxg-0002bW-PD for qemu-devel@nongnu.org; Fri, 02 Sep 2011 09:39:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QzTxf-0004AU-Jo for qemu-devel@nongnu.org; Fri, 02 Sep 2011 09:39:12 -0400 Received: from mx1.redhat.com ([209.132.183.28]:24570) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QzTxf-0004AL-BV for qemu-devel@nongnu.org; Fri, 02 Sep 2011 09:39:11 -0400 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p82Dd8Mn013073 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 2 Sep 2011 09:39:09 -0400 Received: from rincewind.home.kraxel.org (ovpn-116-17.ams2.redhat.com [10.36.116.17]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id p82Dd46S001487; Fri, 2 Sep 2011 09:39:05 -0400 Message-ID: <4E60DC77.5020300@redhat.com> Date: Fri, 02 Sep 2011 15:39:03 +0200 From: Gerd Hoffmann User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.20) Gecko/20110805 Red Hat/3.1.12-1.el6_1 Thunderbird/3.1.12 MIME-Version: 1.0 To: Anthony Liguori References: <20110901163545.71ba1515@doriath> <4E6032AB.8080804@codemonkey.ws> In-Reply-To: <4E6032AB.8080804@codemonkey.ws> X-Scanned-By: MIMEDefang 2.67 on 10.5.11.11 X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 209.132.183.28 Cc: Marian Krcmarik , Alon Levy , qemu-devel , spice-devel , Luiz Capitulino Subject: Re: [Qemu-devel] [PATCH] monitor: Protect outbuf from concurrent access X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Hi, >> After some investigation, I found out that the problem is that different >> SPICE threads are calling monitor functions (such as >> monitor_protocol_event()) in parallel which causes concurrent access >> to the monitor's internal buffer outbuf[]. [ adding spice-list to Cc, see qemu-devel for the rest of the thread ] spice isn't supposed to do that. /me just added a assert in channel_event() and saw it trigger in display channel disconnects. #0 0x0000003ceba32a45 in raise () from /lib64/libc.so.6 #1 0x0000003ceba34225 in abort () from /lib64/libc.so.6 #2 0x0000003ceba2b9d5 in __assert_fail () from /lib64/libc.so.6 #3 0x0000000000503759 in channel_event (event=3, info=0x35e9340) at /home/kraxel/projects/qemu/ui/spice-core.c:223 #4 0x00007f9a77a9921b in reds_channel_event (s=0x35e92c0) at reds.c:400 #5 reds_stream_free (s=0x35e92c0) at reds.c:4981 #6 0x00007f9a77aac8b0 in red_disconnect_channel (channel=0x7f9a24069a80) at red_worker.c:8489 #7 0x00007f9a77ab53a8 in handle_dev_input (listener=0x7f9a3211ab20, events=) at red_worker.c:10062 #8 0x00007f9a77ab436d in red_worker_main (arg=) at red_worker.c:10304 #9 0x0000003cec2077e1 in start_thread () from /lib64/libpthread.so.0 #10 0x0000003cebae68ed in clone () from /lib64/libc.so.6 IMHO spice server should handle the display channel tear-down in the dispatcher instead of the worker thread. Alon? >> Anyways, this commit fixes the problem at hand. Not really. channel_event() itself isn't thread-safe too, it does unlocked list operations which can also blow up when called from different threads. A patch like the attached (warning: untested) should do as quick&dirty fix for stable. But IMO we really should fix spice instead. cheers, Gerd From 7496e573ff6085d3c42d7e65b72c85fd2a7b4a78 Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Fri, 2 Sep 2011 15:03:28 +0200 Subject: [PATCH] spice: workaround a spice server bug. --- ui/spice-core.c | 21 ++++++++++++++++++++- 1 files changed, 20 insertions(+), 1 deletions(-) diff --git a/ui/spice-core.c b/ui/spice-core.c index dba11f0..c99cdc5 100644 --- a/ui/spice-core.c +++ b/ui/spice-core.c @@ -19,6 +19,7 @@ #include #include +#include #include "qemu-common.h" #include "qemu-spice.h" @@ -44,6 +45,8 @@ static char *auth_passwd; static time_t auth_expires = TIME_MAX; int using_spice = 0; +static pthread_t me; + struct SpiceTimer { QEMUTimer *timer; QTAILQ_ENTRY(SpiceTimer) next; @@ -216,6 +219,8 @@ static void channel_event(int event, SpiceChannelEventInfo *info) }; QDict *server, *client; QObject *data; + bool need_lock = !pthread_equal(me, pthread_self()); + static int first = 1; client = qdict_new(); add_addr_info(client, &info->paddr, info->plen); @@ -223,6 +228,14 @@ static void channel_event(int event, SpiceChannelEventInfo *info) server = qdict_new(); add_addr_info(server, &info->laddr, info->llen); + if (need_lock) { + qemu_mutex_lock_iothread(); + if (first) { + fprintf(stderr, "You are using a broken spice-server version\n"); + first = 0; + } + } + if (event == SPICE_CHANNEL_EVENT_INITIALIZED) { qdict_put(server, "auth", qstring_from_str(auth)); add_channel_info(client, info); @@ -236,6 +249,10 @@ static void channel_event(int event, SpiceChannelEventInfo *info) QOBJECT(client), QOBJECT(server)); monitor_protocol_event(qevent[event], data); qobject_decref(data); + + if (need_lock) { + qemu_mutex_unlock_iothread(); + } } #else /* SPICE_INTERFACE_CORE_MINOR >= 3 */ @@ -482,7 +499,9 @@ void qemu_spice_init(void) spice_image_compression_t compression; spice_wan_compression_t wan_compr; - if (!opts) { + me = pthread_self(); + + if (!opts) { return; } port = qemu_opt_get_number(opts, "port", 0);