From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:59376) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T1Xks-0006w0-HN for qemu-devel@nongnu.org; Wed, 15 Aug 2012 03:11:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T1Xkr-0000Py-7W for qemu-devel@nongnu.org; Wed, 15 Aug 2012 03:11:02 -0400 Received: from mail.profihost.ag ([85.158.179.208]:51368) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T1Xkq-0000Pk-St for qemu-devel@nongnu.org; Wed, 15 Aug 2012 03:11:01 -0400 Message-ID: <502B4B7C.6090305@profihost.ag> Date: Wed, 15 Aug 2012 09:10:52 +0200 From: Stefan Priebe - Profihost AG MIME-Version: 1.0 References: <1345014594-6020-1-git-send-email-s.priebe@profihost.ag> <1345014594-6020-2-git-send-email-s.priebe@profihost.ag> In-Reply-To: <1345014594-6020-2-git-send-email-s.priebe@profihost.ag> Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] PATCH V2: fix NULL dereferences / races between task completition and abort List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Priebe Cc: qemu-devel@nongnu.org, ronniesahlberg@gmail.com This patch fixes two main issues with block/iscsi.c: 1.) iscsi_task_mgmt_abort_task_async calls iscsi_scsi_task_cancel which was also directly called in iscsi_aio_cancel 2.) a race between task completition and task abortion could happen cause the scsi_free_scsi_task were done before iscsi_schedule_bh has finished GIT: [PATCH] PATCH V2: fix NULL dereferences / races between task completition and abort Am 15.08.2012 09:09, schrieb Stefan Priebe: > Signed-off-by: Stefan Priebe > > --- > block/iscsi.c | 55 +++++++++++++++++++++++-------------------------------- > 1 files changed, 23 insertions(+), 32 deletions(-) > > diff --git a/block/iscsi.c b/block/iscsi.c > index 12ca76d..1c8b049 100644 > --- a/block/iscsi.c > +++ b/block/iscsi.c > @@ -76,6 +76,10 @@ static void > iscsi_abort_task_cb(struct iscsi_context *iscsi, int status, void *command_data, > void *private_data) > { > + IscsiAIOCB *acb = (IscsiAIOCB *)private_data; > + > + scsi_free_scsi_task(acb->task); > + acb->task = NULL; > } > > static void > @@ -84,15 +88,15 @@ iscsi_aio_cancel(BlockDriverAIOCB *blockacb) > IscsiAIOCB *acb = (IscsiAIOCB *)blockacb; > IscsiLun *iscsilun = acb->iscsilun; > > - acb->common.cb(acb->common.opaque, -ECANCELED); > acb->canceled = 1; > > - /* send a task mgmt call to the target to cancel the task on the target */ > - iscsi_task_mgmt_abort_task_async(iscsilun->iscsi, acb->task, > - iscsi_abort_task_cb, NULL); > + acb->common.cb(acb->common.opaque, -ECANCELED); > > - /* then also cancel the task locally in libiscsi */ > - iscsi_scsi_task_cancel(iscsilun->iscsi, acb->task); > + /* send a task mgmt call to the target to cancel the task on the target > + * this also cancels the task in libiscsi > + */ > + iscsi_task_mgmt_abort_task_async(iscsilun->iscsi, acb->task, > + iscsi_abort_task_cb, &acb); > } > > static AIOPool iscsi_aio_pool = { > @@ -179,11 +183,18 @@ iscsi_readv_writev_bh_cb(void *p) > > qemu_bh_delete(acb->bh); > > - if (acb->canceled == 0) { > + if (!acb->canceled) { > acb->common.cb(acb->common.opaque, acb->status); > } > > qemu_aio_release(acb); > + > + if (acb->canceled) { > + return; > + } > + > + scsi_free_scsi_task(acb->task); > + acb->task = NULL; > } > > > @@ -197,10 +208,8 @@ iscsi_aio_write16_cb(struct iscsi_context *iscsi, int status, > > g_free(acb->buf); > > - if (acb->canceled != 0) { > + if (acb->canceled) { > qemu_aio_release(acb); > - scsi_free_scsi_task(acb->task); > - acb->task = NULL; > return; > } > > @@ -212,8 +221,6 @@ iscsi_aio_write16_cb(struct iscsi_context *iscsi, int status, > } > > iscsi_schedule_bh(iscsi_readv_writev_bh_cb, acb); > - scsi_free_scsi_task(acb->task); > - acb->task = NULL; > } > > static int64_t sector_qemu2lun(int64_t sector, IscsiLun *iscsilun) > @@ -298,10 +305,8 @@ iscsi_aio_read16_cb(struct iscsi_context *iscsi, int status, > > trace_iscsi_aio_read16_cb(iscsi, status, acb, acb->canceled); > > - if (acb->canceled != 0) { > + if (acb->canceled) { > qemu_aio_release(acb); > - scsi_free_scsi_task(acb->task); > - acb->task = NULL; > return; > } > > @@ -313,8 +318,6 @@ iscsi_aio_read16_cb(struct iscsi_context *iscsi, int status, > } > > iscsi_schedule_bh(iscsi_readv_writev_bh_cb, acb); > - scsi_free_scsi_task(acb->task); > - acb->task = NULL; > } > > static BlockDriverAIOCB * > @@ -414,10 +417,8 @@ iscsi_synccache10_cb(struct iscsi_context *iscsi, int status, > { > IscsiAIOCB *acb = opaque; > > - if (acb->canceled != 0) { > + if (acb->canceled) { > qemu_aio_release(acb); > - scsi_free_scsi_task(acb->task); > - acb->task = NULL; > return; > } > > @@ -429,8 +430,6 @@ iscsi_synccache10_cb(struct iscsi_context *iscsi, int status, > } > > iscsi_schedule_bh(iscsi_readv_writev_bh_cb, acb); > - scsi_free_scsi_task(acb->task); > - acb->task = NULL; > } > > static BlockDriverAIOCB * > @@ -468,10 +467,8 @@ iscsi_unmap_cb(struct iscsi_context *iscsi, int status, > { > IscsiAIOCB *acb = opaque; > > - if (acb->canceled != 0) { > + if (acb->canceled) { > qemu_aio_release(acb); > - scsi_free_scsi_task(acb->task); > - acb->task = NULL; > return; > } > > @@ -483,8 +480,6 @@ iscsi_unmap_cb(struct iscsi_context *iscsi, int status, > } > > iscsi_schedule_bh(iscsi_readv_writev_bh_cb, acb); > - scsi_free_scsi_task(acb->task); > - acb->task = NULL; > } > > static BlockDriverAIOCB * > @@ -528,10 +523,8 @@ iscsi_aio_ioctl_cb(struct iscsi_context *iscsi, int status, > { > IscsiAIOCB *acb = opaque; > > - if (acb->canceled != 0) { > + if (acb->canceled) { > qemu_aio_release(acb); > - scsi_free_scsi_task(acb->task); > - acb->task = NULL; > return; > } > > @@ -560,8 +553,6 @@ iscsi_aio_ioctl_cb(struct iscsi_context *iscsi, int status, > } > > iscsi_schedule_bh(iscsi_readv_writev_bh_cb, acb); > - scsi_free_scsi_task(acb->task); > - acb->task = NULL; > } > > static BlockDriverAIOCB *iscsi_aio_ioctl(BlockDriverState *bs, >