From patchwork Mon Aug 20 14:13:09 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Paolo Bonzini X-Patchwork-Id: 178820 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 901492C00A6 for ; Tue, 21 Aug 2012 00:13:45 +1000 (EST) Received: from localhost ([::1]:49904 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T3Sjf-0007Xf-7Z for incoming@patchwork.ozlabs.org; Mon, 20 Aug 2012 10:13:43 -0400 Received: from eggs.gnu.org ([208.118.235.92]:34199) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T3SjP-0007WY-20 for qemu-devel@nongnu.org; Mon, 20 Aug 2012 10:13:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T3SjJ-0007CD-2h for qemu-devel@nongnu.org; Mon, 20 Aug 2012 10:13:27 -0400 Received: from mail-ey0-f173.google.com ([209.85.215.173]:61264) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T3SjI-0007Bz-PM for qemu-devel@nongnu.org; Mon, 20 Aug 2012 10:13:21 -0400 Received: by eaac13 with SMTP id c13so1910510eaa.4 for ; Mon, 20 Aug 2012 07:13:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:from:to:cc:subject:date:message-id:x-mailer:in-reply-to :references; bh=rcfmOjsJun1WLtRLFcVecsKQYVBtrmHwNbPMZyp7pcE=; b=VuFd9UJAI5ScB7SwdoPPrdSLNTH0KDS1hh2ct5zYVQ6fALyTQ2jMzR1brMy8906Jhb FTKTJ78JIg54q4zSX3l9HqIYrmYaIs0vmYZxoI+Ws6Na4823hZZVRTfNMC290wpsYq82 GJT96M/pd5XVnvf3/9qeeeGzL65MIt8r6IzisQFAyZqTF8+7A+NNE3mBosU4iDUPsxwL 6NLIfNrA7Mhg5LZMcjUQgzR0x4QMVbGbk6Elsbh3N69ZeezAfx227eor7Kd4fODwnAqs OGVF/JfZCwuvqXtKiFdRzNTFX1mciXzVsHX6hYx4zF/J+SBFygNeR1LNOOQaGG5/1VMp avaw== Received: by 10.14.182.9 with SMTP id n9mr8526575eem.6.1345472000056; Mon, 20 Aug 2012 07:13:20 -0700 (PDT) Received: from yakj.usersys.redhat.com (nat-pool-mxp-t.redhat.com. [209.132.186.18]) by mx.google.com with ESMTPS id 45sm42202715eed.17.2012.08.20.07.13.19 (version=TLSv1/SSLv3 cipher=OTHER); Mon, 20 Aug 2012 07:13:19 -0700 (PDT) From: Paolo Bonzini To: qemu-devel@nongnu.org Date: Mon, 20 Aug 2012 16:13:09 +0200 Message-Id: <1345471993-31629-2-git-send-email-pbonzini@redhat.com> X-Mailer: git-send-email 1.7.11.2 In-Reply-To: <1345471993-31629-1-git-send-email-pbonzini@redhat.com> References: <1345471993-31629-1-git-send-email-pbonzini@redhat.com> X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 209.85.215.173 Cc: anthony@codemonkey.ws Subject: [Qemu-devel] [PATCH 1/5] Revert "iscsi: Fix NULL dereferences / races between task completion and abort" X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org This reverts commit 64e69e80920d82df3fa679bc41b13770d2f99360. The commit returned immediately from iscsi_aio_cancel, risking corruption in case the following happens: guest qemu target ========================================================================= send write 1 --------> send write 1 --------> cancel write 1 ------> cancel write 1 ------> <------------------ cancellation processed send write 2 --------> send write 2 --------> <---------------- completed write 2 <------------------ completed write 2 <---------------- completed write 1 <---------------- cancellation not done Here, the guest would see write 2 superseding write 1, when in fact the outcome could have been the opposite. The right behavior is to return only after the target says whether the cancellation was done or not, and it will be implemented by the next three patches. Signed-off-by: Paolo Bonzini --- block/iscsi.c | 55 ++++++++++++++++++++++++++++++++----------------------- 1 file modificato, 32 inserzioni(+), 23 rimozioni(-) diff --git a/block/iscsi.c b/block/iscsi.c index bb9cf82..219f927 100644 --- a/block/iscsi.c +++ b/block/iscsi.c @@ -76,10 +76,6 @@ 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 @@ -88,15 +84,15 @@ iscsi_aio_cancel(BlockDriverAIOCB *blockacb) IscsiAIOCB *acb = (IscsiAIOCB *)blockacb; IscsiLun *iscsilun = acb->iscsilun; - acb->canceled = 1; - 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 - * this also cancels the task in libiscsi - */ + /* 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, &acb); + iscsi_abort_task_cb, NULL); + + /* then also cancel the task locally in libiscsi */ + iscsi_scsi_task_cancel(iscsilun->iscsi, acb->task); } static AIOPool iscsi_aio_pool = { @@ -183,18 +179,11 @@ iscsi_readv_writev_bh_cb(void *p) qemu_bh_delete(acb->bh); - if (!acb->canceled) { + if (acb->canceled == 0) { 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; } @@ -208,8 +197,10 @@ iscsi_aio_write16_cb(struct iscsi_context *iscsi, int status, g_free(acb->buf); - if (acb->canceled) { + if (acb->canceled != 0) { qemu_aio_release(acb); + scsi_free_scsi_task(acb->task); + acb->task = NULL; return; } @@ -221,6 +212,8 @@ 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) @@ -305,8 +298,10 @@ iscsi_aio_read16_cb(struct iscsi_context *iscsi, int status, trace_iscsi_aio_read16_cb(iscsi, status, acb, acb->canceled); - if (acb->canceled) { + if (acb->canceled != 0) { qemu_aio_release(acb); + scsi_free_scsi_task(acb->task); + acb->task = NULL; return; } @@ -318,6 +313,8 @@ 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 * @@ -417,8 +414,10 @@ iscsi_synccache10_cb(struct iscsi_context *iscsi, int status, { IscsiAIOCB *acb = opaque; - if (acb->canceled) { + if (acb->canceled != 0) { qemu_aio_release(acb); + scsi_free_scsi_task(acb->task); + acb->task = NULL; return; } @@ -430,6 +429,8 @@ 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 * @@ -467,8 +468,10 @@ iscsi_unmap_cb(struct iscsi_context *iscsi, int status, { IscsiAIOCB *acb = opaque; - if (acb->canceled) { + if (acb->canceled != 0) { qemu_aio_release(acb); + scsi_free_scsi_task(acb->task); + acb->task = NULL; return; } @@ -480,6 +483,8 @@ 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 * @@ -523,8 +528,10 @@ iscsi_aio_ioctl_cb(struct iscsi_context *iscsi, int status, { IscsiAIOCB *acb = opaque; - if (acb->canceled) { + if (acb->canceled != 0) { qemu_aio_release(acb); + scsi_free_scsi_task(acb->task); + acb->task = NULL; return; } @@ -553,6 +560,8 @@ 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,