From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45251) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X2ITR-0002L5-Vw for qemu-devel@nongnu.org; Wed, 02 Jul 2014 07:13:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X2ITK-0005Z3-0n for qemu-devel@nongnu.org; Wed, 02 Jul 2014 07:13:13 -0400 Received: from szxga02-in.huawei.com ([119.145.14.65]:50081) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X2ITJ-0005Yj-9S for qemu-devel@nongnu.org; Wed, 02 Jul 2014 07:13:05 -0400 Message-ID: <53B3E92E.1040909@huawei.com> Date: Wed, 2 Jul 2014 19:12:46 +0800 From: ChenLiang MIME-Version: 1.0 References: <1404291017-7456-1-git-send-email-arei.gonglei@huawei.com> <53B3CB04.5040909@redhat.com> <53B3CFB8.5000800@huawei.com> <53B3D011.9000200@redhat.com> <33183CC9F5247A488A2544077AF1902086C18093@SZXEMA503-MBS.china.huawei.com> <53B3DBFD.1090009@redhat.com> In-Reply-To: <53B3DBFD.1090009@redhat.com> Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] ide: fix double free List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: "kwolf@redhat.com" , "Gonglei (Arei)" , "Huangweidong (C)" , "qemu-devel@nongnu.org" , "stefanha@redhat.com" On 2014/7/2 18:16, Paolo Bonzini wrote: > Il 02/07/2014 11:46, Gonglei (Arei) ha scritto: >> Hi, Paolo. We have tested your above patch, and it works well for us. > > I'm still not sure where the fix is. I jotted the patch quickly, but I'd rather understand it better before submitting it. Here is it again: > > --- a/dma-helpers.c > +++ b/dma-helpers.c > @@ -181,15 +181,15 @@ static void dma_aio_cancel(BlockDriverAIOCB *acb) > trace_dma_aio_cancel(dbs); > > + dbs->in_cancel = true; > if (dbs->acb) { > BlockDriverAIOCB *acb = dbs->acb; > dbs->acb = NULL; > - dbs->in_cancel = true; > bdrv_aio_cancel(acb); > - dbs->in_cancel = false; > } > dbs->common.cb = NULL; > dma_complete(dbs, 0); > + qemu_aio_release(dbs); > } > > static const AIOCBInfo dma_aiocb_info = { > > and here is dma_complete: > > static void dma_complete(DMAAIOCB *dbs, int ret) > { > trace_dma_complete(dbs, ret, dbs->common.cb); > dma_bdrv_unmap(dbs); > if (dbs->common.cb) { > dbs->common.cb(dbs->common.opaque, ret); > } > qemu_iovec_destroy(&dbs->iov); > if (dbs->bh) { > qemu_bh_delete(dbs->bh); > dbs->bh = NULL; > } > if (!dbs->in_cancel) { > /* Requests may complete while dma_aio_cancel is in progress. > * this case, the AIOCB should not be released because it is > * still referenced by dma_aio_cancel. */ > qemu_aio_release(dbs); > } > } > > The qemu_aio_release call obviously happened during the second dma_complete call. > > However, the second dma_complete call does not call the callback, and even if dma_bdrv_unmap resulted in a call to continue_after_map_failure, the bottom half would be cancelled immediately. So we changed from: > > dbs->in_cancel = false; > > /* dma_complete calls dma_bdrv_unmap */ > for (i = 0; i < dbs->iov.niov; ++i) { > /* might schedule dbs->bh */ > dma_memory_unmap(dbs->sg->as, dbs->iov.iov[i].iov_base, > dbs->iov.iov[i].iov_len, dbs->dir, > dbs->iov.iov[i].iov_len); > } > qemu_iovec_reset(&dbs->iov); > > /* back to dma_complete, dbs->common.cb == NULL */ > qemu_iovec_destroy(&dbs->iov); > if (dbs->bh) { > /* dbs->bh now unscheduled, so it will never run */ > qemu_bh_delete(dbs->bh); > dbs->bh = NULL; > } > /* dbs->in_cancel is false here. */ > if (!dbs->in_cancel) { > /* Requests may complete while dma_aio_cancel is in progress. > * this case, the AIOCB should not be released because it is > * still referenced by dma_aio_cancel. */ > qemu_aio_release(dbs); > } > > to > > dbs->in_cancel = false; > > /* dma_complete calls dma_bdrv_unmap */ > for (i = 0; i < dbs->iov.niov; ++i) { > /* might schedule dbs->bh */ > dma_memory_unmap(dbs->sg->as, dbs->iov.iov[i].iov_base, > dbs->iov.iov[i].iov_len, dbs->dir, > dbs->iov.iov[i].iov_len); > } > qemu_iovec_reset(&dbs->iov); > > /* back to dma_complete, dbs->common.cb == NULL */ > qemu_iovec_destroy(&dbs->iov); > if (dbs->bh) { > /* dbs->bh now unscheduled, so it will never run */ > qemu_bh_delete(dbs->bh); > dbs->bh = NULL; > } > > /* after the patch dbs->in_cancel is true, the if never triggers */ > if (!dbs->in_cancel) { > /* Requests may complete while dma_aio_cancel is in progress. > * this case, the AIOCB should not be released because it is > * still referenced by dma_aio_cancel. */ > qemu_aio_release(dbs); > } > > /* back to dma_aio_cancel */ > qemu_aio_release(dbs); > > This doesn't make sense to me. Can you find out where I'm wrong? > > Paolo > > . > This patch avoid freeing dbs by dma_complete when dma_aio_cancel is running. Because dma_complete also will be called by dma_bdrv_cb. So double free will never happen. Chenliang