From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51973) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1agZWR-0005Vi-UD for qemu-devel@nongnu.org; Thu, 17 Mar 2016 11:07:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1agZWR-0003O0-3M for qemu-devel@nongnu.org; Thu, 17 Mar 2016 11:07:35 -0400 References: <1458123018-18651-1-git-send-email-famz@redhat.com> <1458123018-18651-5-git-send-email-famz@redhat.com> <20160317150057.GP14062@stefanha-x1.localdomain> From: Paolo Bonzini Message-ID: <56EAC82A.50907@redhat.com> Date: Thu, 17 Mar 2016 16:07:22 +0100 MIME-Version: 1.0 In-Reply-To: <20160317150057.GP14062@stefanha-x1.localdomain> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [Qemu-block] [PATCH 4/4] virtio-blk: Clean up start/stop with mutex and BH List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi , Fam Zheng Cc: Kevin Wolf , qemu-block@nongnu.org, "Michael S. Tsirkin" , qemu-devel@nongnu.org, tubo@linux.vnet.ibm.com, Stefan Hajnoczi , cornelia.huck@de.ibm.com, borntraeger@de.ibm.com On 17/03/2016 16:00, Stefan Hajnoczi wrote: >> > + data =3D g_new(VirtIOBlockStartData, 1); >> > + data->vblk =3D vblk; >> > + data->bh =3D aio_bh_new(s->ctx, virtio_blk_data_plane_start_bh_= cb, data); >> > + qemu_bh_schedule(data->bh); >> > + qemu_mutex_unlock(&s->start_stop_lock); >> > return; > This BH usage pattern is dangerous: >=20 > 1. The BH is created and scheduled. > 2. Before the BH executes the device is unrealized. > 3. The data->bh pointer is inaccessible so we have a dangling BH that > will access vblk after vblk has been freed. >=20 > In some cases it can be safe but I don't see why the pattern is safe in > this case. Either the BH needs to hold some sort of reference to keep > vblk alive, or vblk needs to know about pending BHs so they can be > deleted. You're right. After unrealizing virtio_blk_data_plane_stop has set of vblk->dataplane_started =3D false, so that's covered. However, you still need an object_ref/object_object_unref pair. That said, Christian wasn't testing hotplug/hot-unplug so this shouldn't matter in his case. Let's see if we can catch the reentrancy with an assertion... Paolo