From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33917) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X2Hal-0000ad-CQ for qemu-devel@nongnu.org; Wed, 02 Jul 2014 06:16:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X2Hac-0005HW-7I for qemu-devel@nongnu.org; Wed, 02 Jul 2014 06:16:43 -0400 Received: from mail-qg0-x22c.google.com ([2607:f8b0:400d:c04::22c]:61939) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X2Hac-0005H1-1N for qemu-devel@nongnu.org; Wed, 02 Jul 2014 06:16:34 -0400 Received: by mail-qg0-f44.google.com with SMTP id j107so4547056qga.31 for ; Wed, 02 Jul 2014 03:16:33 -0700 (PDT) Sender: Paolo Bonzini Message-ID: <53B3DBFD.1090009@redhat.com> Date: Wed, 02 Jul 2014 12:16:29 +0200 From: Paolo Bonzini 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> In-Reply-To: <33183CC9F5247A488A2544077AF1902086C18093@SZXEMA503-MBS.china.huawei.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed 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: "Gonglei (Arei)" , "chenliang (T)" Cc: "kwolf@redhat.com" , "Huangweidong (C)" , "qemu-devel@nongnu.org" , "stefanha@redhat.com" 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