From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43060) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bx8oF-0006bo-6O for qemu-devel@nongnu.org; Thu, 20 Oct 2016 04:34:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bx8oC-0007Ji-HF for qemu-devel@nongnu.org; Thu, 20 Oct 2016 04:34:43 -0400 Received: from mx1.redhat.com ([209.132.183.28]:36352) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1bx8oC-0007Ic-AO for qemu-devel@nongnu.org; Thu, 20 Oct 2016 04:34:40 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 9C6BEC0641E9 for ; Thu, 20 Oct 2016 08:34:39 +0000 (UTC) Date: Thu, 20 Oct 2016 09:34:36 +0100 From: "Daniel P. Berrange" Message-ID: <20161020083436.GB12145@redhat.com> Reply-To: "Daniel P. Berrange" References: <20161018100409.GH4349@redhat.com> <20161018113202.GE2190@work-vm> <20161018120121.GN4349@redhat.com> <20161018132524.GG2190@work-vm> <20161018133528.GD12728@redhat.com> <20161018135213.GI2190@work-vm> <20161018140141.GF12728@redhat.com> <20161019170148.GE2035@work-vm> <1aa4a32c-3816-1c8d-6722-b54c9f22fe09@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1aa4a32c-3816-1c8d-6722-b54c9f22fe09@redhat.com> Subject: Re: [Qemu-devel] chardev's and fd's in monitors List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: "Dr. David Alan Gilbert" , qemu-devel@nongnu.org, armbru@redhat.com On Wed, Oct 19, 2016 at 10:51:07PM +0200, Paolo Bonzini wrote: > > > On 19/10/2016 19:01, Dr. David Alan Gilbert wrote: > > * Paolo Bonzini (pbonzini@redhat.com) wrote: > >> > >> > >> On 18/10/2016 16:01, Daniel P. Berrange wrote: > >>>>> > >>>>> I already use error_report's in places in migration threads of various > >>>>> types; I'm not sure if that's a problem. > >>> Unless those places are protected by the big qemu lock, that sounds > >>> not good. error_report calls into error_vprintf which checks the > >>> 'cur_mon' global "Monitor" pointer. This variable is updated at > >>> runtime - eg in qmp_human_monitor_command(), monitor_qmp_read(), > >>> monitor_read(), etc. So if migration threads outside the BQL are > >>> calling error_report() that could well cause problems. If you > >>> are lucky messages will merely end up going to stderr instead of > >>> the monitor, but in worst case I wouldn't be surprised if there > >>> is a crash possibility in some race conditions. > >> > >> Writes to chardevs *are* thread-safe (assuming qio_channel_create_watch > >> is thread-safe; it seems to be). > > > > Hmm that's useful (although it doesn't solve error_report because error_vprintf > > is racy itself). > > How is it racy? Because of the case where cur_mon changes under the > feet of error_vprintf? I guess that can be ignored for now (just a TODO > comment will do). Yes, the usage of cur_mon is unsafe as there's a TOCTOU race in there that I believe can result in a NULL pointer crash. eg these two methods: static inline bool monitor_is_qmp(const Monitor *mon) { return (mon->flags & MONITOR_USE_CONTROL); } bool monitor_cur_is_qmp(void) { return cur_mon && monitor_is_qmp(cur_mon); } In using error_vprintf() from a non-main thread and not holding the big QEMU lock, then 'cur_mon' can change between time we check that its non-NULL, and when accessing cur_mon->flags. Admittedly its a rare race - you could have to have an error being reported by background migration, concurrently with a monitor command being invoked, but that will hit someone eventually unless I'm missing some synchronization somewhere. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|