From b20909195745c34a819aed14ae996b60ab0f591f Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Sun, 19 Aug 2012 00:12:39 +0200 Subject: Revert "iscsi: Fix NULL dereferences / races between task completion and abort" 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 changed, 32 insertions(+), 23 deletions(-) diff --git a/block/iscsi.c b/block/iscsi.c index bb9cf82459..219f927823 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, -- cgit v1.2.3 From 27cbd828c617944c0f9603763fdf4fa87e7ad923 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Sat, 18 Aug 2012 23:37:31 +0200 Subject: iscsi: move iscsi_schedule_bh and iscsi_readv_writev_bh_cb Put these functions at the beginning, to avoid forward references in the next patches. Signed-off-by: Paolo Bonzini --- block/iscsi.c | 56 ++++++++++++++++++++++++++++---------------------------- 1 file changed, 28 insertions(+), 28 deletions(-) diff --git a/block/iscsi.c b/block/iscsi.c index 219f927823..600e334e13 100644 --- a/block/iscsi.c +++ b/block/iscsi.c @@ -72,6 +72,34 @@ struct IscsiTask { int complete; }; +static void +iscsi_readv_writev_bh_cb(void *p) +{ + IscsiAIOCB *acb = p; + + qemu_bh_delete(acb->bh); + + if (acb->canceled == 0) { + acb->common.cb(acb->common.opaque, acb->status); + } + + qemu_aio_release(acb); +} + +static int +iscsi_schedule_bh(QEMUBHFunc *cb, IscsiAIOCB *acb) +{ + acb->bh = qemu_bh_new(cb, acb); + if (!acb->bh) { + error_report("oom: could not create iscsi bh"); + return -EIO; + } + + qemu_bh_schedule(acb->bh); + return 0; +} + + static void iscsi_abort_task_cb(struct iscsi_context *iscsi, int status, void *command_data, void *private_data) @@ -159,34 +187,6 @@ iscsi_process_write(void *arg) } -static int -iscsi_schedule_bh(QEMUBHFunc *cb, IscsiAIOCB *acb) -{ - acb->bh = qemu_bh_new(cb, acb); - if (!acb->bh) { - error_report("oom: could not create iscsi bh"); - return -EIO; - } - - qemu_bh_schedule(acb->bh); - return 0; -} - -static void -iscsi_readv_writev_bh_cb(void *p) -{ - IscsiAIOCB *acb = p; - - qemu_bh_delete(acb->bh); - - if (acb->canceled == 0) { - acb->common.cb(acb->common.opaque, acb->status); - } - - qemu_aio_release(acb); -} - - static void iscsi_aio_write16_cb(struct iscsi_context *iscsi, int status, void *command_data, void *opaque) -- cgit v1.2.3 From cfb3f5064af2d2e29c976e292c9472dfe9d61e31 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Sat, 18 Aug 2012 23:38:03 +0200 Subject: iscsi: simplify iscsi_schedule_bh It is always used with the same callback, remove the argument. And its return value is never used, assume allocation succeeds. Signed-off-by: Paolo Bonzini --- block/iscsi.c | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/block/iscsi.c b/block/iscsi.c index 600e334e13..7b09795720 100644 --- a/block/iscsi.c +++ b/block/iscsi.c @@ -73,7 +73,7 @@ struct IscsiTask { }; static void -iscsi_readv_writev_bh_cb(void *p) +iscsi_bh_cb(void *p) { IscsiAIOCB *acb = p; @@ -86,17 +86,11 @@ iscsi_readv_writev_bh_cb(void *p) qemu_aio_release(acb); } -static int -iscsi_schedule_bh(QEMUBHFunc *cb, IscsiAIOCB *acb) +static void +iscsi_schedule_bh(IscsiAIOCB *acb) { - acb->bh = qemu_bh_new(cb, acb); - if (!acb->bh) { - error_report("oom: could not create iscsi bh"); - return -EIO; - } - + acb->bh = qemu_bh_new(iscsi_bh_cb, acb); qemu_bh_schedule(acb->bh); - return 0; } @@ -211,7 +205,7 @@ iscsi_aio_write16_cb(struct iscsi_context *iscsi, int status, acb->status = -EIO; } - iscsi_schedule_bh(iscsi_readv_writev_bh_cb, acb); + iscsi_schedule_bh(acb); scsi_free_scsi_task(acb->task); acb->task = NULL; } @@ -312,7 +306,7 @@ iscsi_aio_read16_cb(struct iscsi_context *iscsi, int status, acb->status = -EIO; } - iscsi_schedule_bh(iscsi_readv_writev_bh_cb, acb); + iscsi_schedule_bh(acb); scsi_free_scsi_task(acb->task); acb->task = NULL; } @@ -428,7 +422,7 @@ iscsi_synccache10_cb(struct iscsi_context *iscsi, int status, acb->status = -EIO; } - iscsi_schedule_bh(iscsi_readv_writev_bh_cb, acb); + iscsi_schedule_bh(acb); scsi_free_scsi_task(acb->task); acb->task = NULL; } @@ -482,7 +476,7 @@ iscsi_unmap_cb(struct iscsi_context *iscsi, int status, acb->status = -EIO; } - iscsi_schedule_bh(iscsi_readv_writev_bh_cb, acb); + iscsi_schedule_bh(acb); scsi_free_scsi_task(acb->task); acb->task = NULL; } @@ -559,7 +553,7 @@ iscsi_aio_ioctl_cb(struct iscsi_context *iscsi, int status, memcpy(acb->ioh->sbp, &acb->task->datain.data[2], ss); } - iscsi_schedule_bh(iscsi_readv_writev_bh_cb, acb); + iscsi_schedule_bh(acb); scsi_free_scsi_task(acb->task); acb->task = NULL; } -- cgit v1.2.3 From 1bd075f29ea6d11853475c7c42734595720c3ac6 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Sat, 18 Aug 2012 23:35:49 +0200 Subject: iscsi: fix races between task completion and abort 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 completion and task abortion could happen cause the scsi_free_scsi_task were done before iscsi_schedule_bh has finished. To fix this, all the freeing of IscsiTasks and releasing of the AIOCBs is centralized in iscsi_bh_cb, independent of whether the SCSI command has completed or was cancelled. 3) iscsi_aio_cancel was not synchronously waiting for the end of the command. Signed-off-by: Paolo Bonzini --- block/iscsi.c | 59 ++++++++++++++++++++++++++++++----------------------------- 1 file changed, 30 insertions(+), 29 deletions(-) diff --git a/block/iscsi.c b/block/iscsi.c index 7b09795720..4828b83927 100644 --- a/block/iscsi.c +++ b/block/iscsi.c @@ -83,12 +83,20 @@ iscsi_bh_cb(void *p) acb->common.cb(acb->common.opaque, acb->status); } + if (acb->task != NULL) { + scsi_free_scsi_task(acb->task); + acb->task = NULL; + } + qemu_aio_release(acb); } static void iscsi_schedule_bh(IscsiAIOCB *acb) { + if (acb->bh) { + return; + } acb->bh = qemu_bh_new(iscsi_bh_cb, acb); qemu_bh_schedule(acb->bh); } @@ -98,6 +106,10 @@ static void iscsi_abort_task_cb(struct iscsi_context *iscsi, int status, void *command_data, void *private_data) { + IscsiAIOCB *acb = private_data; + + acb->status = -ECANCELED; + iscsi_schedule_bh(acb); } static void @@ -106,15 +118,19 @@ iscsi_aio_cancel(BlockDriverAIOCB *blockacb) IscsiAIOCB *acb = (IscsiAIOCB *)blockacb; IscsiLun *iscsilun = acb->iscsilun; - acb->common.cb(acb->common.opaque, -ECANCELED); + if (acb->status != -EINPROGRESS) { + return; + } + 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); + iscsi_abort_task_cb, acb); - /* then also cancel the task locally in libiscsi */ - iscsi_scsi_task_cancel(iscsilun->iscsi, acb->task); + while (acb->status == -EINPROGRESS) { + qemu_aio_wait(); + } } static AIOPool iscsi_aio_pool = { @@ -192,9 +208,6 @@ iscsi_aio_write16_cb(struct iscsi_context *iscsi, int status, g_free(acb->buf); if (acb->canceled != 0) { - qemu_aio_release(acb); - scsi_free_scsi_task(acb->task); - acb->task = NULL; return; } @@ -206,8 +219,6 @@ iscsi_aio_write16_cb(struct iscsi_context *iscsi, int status, } iscsi_schedule_bh(acb); - scsi_free_scsi_task(acb->task); - acb->task = NULL; } static int64_t sector_qemu2lun(int64_t sector, IscsiLun *iscsilun) @@ -236,6 +247,8 @@ iscsi_aio_writev(BlockDriverState *bs, int64_t sector_num, acb->qiov = qiov; acb->canceled = 0; + acb->bh = NULL; + acb->status = -EINPROGRESS; /* XXX we should pass the iovec to write16 to avoid the extra copy */ /* this will allow us to get rid of 'buf' completely */ @@ -293,9 +306,6 @@ iscsi_aio_read16_cb(struct iscsi_context *iscsi, int status, trace_iscsi_aio_read16_cb(iscsi, status, acb, acb->canceled); if (acb->canceled != 0) { - qemu_aio_release(acb); - scsi_free_scsi_task(acb->task); - acb->task = NULL; return; } @@ -307,8 +317,6 @@ iscsi_aio_read16_cb(struct iscsi_context *iscsi, int status, } iscsi_schedule_bh(acb); - scsi_free_scsi_task(acb->task); - acb->task = NULL; } static BlockDriverAIOCB * @@ -334,6 +342,8 @@ iscsi_aio_readv(BlockDriverState *bs, int64_t sector_num, acb->qiov = qiov; acb->canceled = 0; + acb->bh = NULL; + acb->status = -EINPROGRESS; acb->read_size = qemu_read_size; acb->buf = NULL; @@ -409,9 +419,6 @@ iscsi_synccache10_cb(struct iscsi_context *iscsi, int status, IscsiAIOCB *acb = opaque; if (acb->canceled != 0) { - qemu_aio_release(acb); - scsi_free_scsi_task(acb->task); - acb->task = NULL; return; } @@ -423,8 +430,6 @@ iscsi_synccache10_cb(struct iscsi_context *iscsi, int status, } iscsi_schedule_bh(acb); - scsi_free_scsi_task(acb->task); - acb->task = NULL; } static BlockDriverAIOCB * @@ -439,6 +444,8 @@ iscsi_aio_flush(BlockDriverState *bs, acb->iscsilun = iscsilun; acb->canceled = 0; + acb->bh = NULL; + acb->status = -EINPROGRESS; acb->task = iscsi_synchronizecache10_task(iscsi, iscsilun->lun, 0, 0, 0, 0, @@ -463,9 +470,6 @@ iscsi_unmap_cb(struct iscsi_context *iscsi, int status, IscsiAIOCB *acb = opaque; if (acb->canceled != 0) { - qemu_aio_release(acb); - scsi_free_scsi_task(acb->task); - acb->task = NULL; return; } @@ -477,8 +481,6 @@ iscsi_unmap_cb(struct iscsi_context *iscsi, int status, } iscsi_schedule_bh(acb); - scsi_free_scsi_task(acb->task); - acb->task = NULL; } static BlockDriverAIOCB * @@ -495,6 +497,8 @@ iscsi_aio_discard(BlockDriverState *bs, acb->iscsilun = iscsilun; acb->canceled = 0; + acb->bh = NULL; + acb->status = -EINPROGRESS; list[0].lba = sector_qemu2lun(sector_num, iscsilun); list[0].num = nb_sectors * BDRV_SECTOR_SIZE / iscsilun->block_size; @@ -523,9 +527,6 @@ iscsi_aio_ioctl_cb(struct iscsi_context *iscsi, int status, IscsiAIOCB *acb = opaque; if (acb->canceled != 0) { - qemu_aio_release(acb); - scsi_free_scsi_task(acb->task); - acb->task = NULL; return; } @@ -554,8 +555,6 @@ iscsi_aio_ioctl_cb(struct iscsi_context *iscsi, int status, } iscsi_schedule_bh(acb); - scsi_free_scsi_task(acb->task); - acb->task = NULL; } static BlockDriverAIOCB *iscsi_aio_ioctl(BlockDriverState *bs, @@ -573,6 +572,8 @@ static BlockDriverAIOCB *iscsi_aio_ioctl(BlockDriverState *bs, acb->iscsilun = iscsilun; acb->canceled = 0; + acb->bh = NULL; + acb->status = -EINPROGRESS; acb->buf = NULL; acb->ioh = buf; -- cgit v1.2.3 From 07a5298c3067ed63f7d82d8c1a6f479146447289 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 20 Aug 2012 15:23:28 +0200 Subject: virtio-scsi: add backwards-compatibility properties for 1.1 and earlier machines Hotplug and parameter change are new in 1.2, disable them via compat properties for pc-1.1 and earlier. Signed-off-by: Paolo Bonzini --- hw/pc_piix.c | 8 ++++++++ hw/virtio-scsi.c | 7 ------- hw/virtio-scsi.h | 9 ++++++++- 3 files changed, 16 insertions(+), 8 deletions(-) diff --git a/hw/pc_piix.c b/hw/pc_piix.c index 0c0096fd7e..a960710af8 100644 --- a/hw/pc_piix.c +++ b/hw/pc_piix.c @@ -360,6 +360,14 @@ static QEMUMachine pc_machine_v1_2 = { #define PC_COMPAT_1_1 \ {\ + .driver = "virtio-scsi-pci",\ + .property = "hotplug",\ + .value = "off",\ + },{\ + .driver = "virtio-scsi-pci",\ + .property = "param_change",\ + .value = "off",\ + },{\ .driver = "VGA",\ .property = "vgamem_mb",\ .value = stringify(8),\ diff --git a/hw/virtio-scsi.c b/hw/virtio-scsi.c index 5f737acd97..c1b47a8f4d 100644 --- a/hw/virtio-scsi.c +++ b/hw/virtio-scsi.c @@ -24,11 +24,6 @@ #define VIRTIO_SCSI_MAX_TARGET 255 #define VIRTIO_SCSI_MAX_LUN 16383 -/* Feature Bits */ -#define VIRTIO_SCSI_F_INOUT 0 -#define VIRTIO_SCSI_F_HOTPLUG 1 -#define VIRTIO_SCSI_F_CHANGE 2 - /* Response codes */ #define VIRTIO_SCSI_S_OK 0 #define VIRTIO_SCSI_S_OVERRUN 1 @@ -561,8 +556,6 @@ static void virtio_scsi_set_config(VirtIODevice *vdev, static uint32_t virtio_scsi_get_features(VirtIODevice *vdev, uint32_t requested_features) { - requested_features |= (1UL << VIRTIO_SCSI_F_HOTPLUG); - requested_features |= (1UL << VIRTIO_SCSI_F_CHANGE); return requested_features; } diff --git a/hw/virtio-scsi.h b/hw/virtio-scsi.h index 4bc889de02..91924f6dfc 100644 --- a/hw/virtio-scsi.h +++ b/hw/virtio-scsi.h @@ -21,6 +21,11 @@ /* The ID for virtio_scsi */ #define VIRTIO_ID_SCSI 8 +/* Feature Bits */ +#define VIRTIO_SCSI_F_INOUT 0 +#define VIRTIO_SCSI_F_HOTPLUG 1 +#define VIRTIO_SCSI_F_CHANGE 2 + struct VirtIOSCSIConf { uint32_t num_queues; uint32_t max_sectors; @@ -31,6 +36,8 @@ struct VirtIOSCSIConf { DEFINE_VIRTIO_COMMON_FEATURES(_state, _features_field), \ DEFINE_PROP_UINT32("num_queues", _state, _conf_field.num_queues, 1), \ DEFINE_PROP_UINT32("max_sectors", _state, _conf_field.max_sectors, 0xFFFF), \ - DEFINE_PROP_UINT32("cmd_per_lun", _state, _conf_field.cmd_per_lun, 128) + DEFINE_PROP_UINT32("cmd_per_lun", _state, _conf_field.cmd_per_lun, 128), \ + DEFINE_PROP_BIT("hotplug", _state, _features_field, VIRTIO_SCSI_F_HOTPLUG, true), \ + DEFINE_PROP_BIT("param_change", _state, _features_field, VIRTIO_SCSI_F_CHANGE, true) #endif /* _QEMU_VIRTIO_SCSI_H */ -- cgit v1.2.3