* [Qemu-devel] [PATCH] PATCH V2: fix NULL dereferences / races between task completition and abort
@ 2012-08-14 20:29 Stefan Priebe
0 siblings, 0 replies; 7+ messages in thread
From: Stefan Priebe @ 2012-08-14 20:29 UTC (permalink / raw)
To: qemu-devel; +Cc: Stefan Priebe
Signed-off-by: Stefan Priebe <s.priebe@profihost.ag>
---
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,
--
1.7.2.5
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [Qemu-devel] [PATCH] PATCH V2: fix NULL dereferences / races between task completition and abort
@ 2012-08-15 7:05 Stefan Priebe
0 siblings, 0 replies; 7+ messages in thread
From: Stefan Priebe @ 2012-08-15 7:05 UTC (permalink / raw)
To: qemu-devel; +Cc: 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,
--
1.7.2.5
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [Qemu-devel] PATCH V2: fix NULL dereferences / races between task completition and abort
@ 2012-08-15 7:09 Stefan Priebe
2012-08-15 7:09 ` [Qemu-devel] [PATCH] " Stefan Priebe
0 siblings, 1 reply; 7+ messages in thread
From: Stefan Priebe @ 2012-08-15 7:09 UTC (permalink / raw)
To: qemu-devel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Qemu-devel] [PATCH] PATCH V2: fix NULL dereferences / races between task completition and abort
2012-08-15 7:09 [Qemu-devel] PATCH V2: fix NULL dereferences / races between task completition and abort Stefan Priebe
@ 2012-08-15 7:09 ` Stefan Priebe
2012-08-15 7:10 ` Stefan Priebe - Profihost AG
2012-08-15 8:13 ` ronnie sahlberg
0 siblings, 2 replies; 7+ messages in thread
From: Stefan Priebe @ 2012-08-15 7:09 UTC (permalink / raw)
To: qemu-devel; +Cc: ronniesahlberg, Stefan Priebe
Signed-off-by: Stefan Priebe <s.priebe@profihost.ag>
---
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,
--
1.7.2.5
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] PATCH V2: fix NULL dereferences / races between task completition and abort
2012-08-15 7:09 ` [Qemu-devel] [PATCH] " Stefan Priebe
@ 2012-08-15 7:10 ` Stefan Priebe - Profihost AG
2012-08-15 8:13 ` ronnie sahlberg
1 sibling, 0 replies; 7+ messages in thread
From: Stefan Priebe - Profihost AG @ 2012-08-15 7:10 UTC (permalink / raw)
To: Stefan Priebe; +Cc: qemu-devel, ronniesahlberg
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 <s.priebe@profihost.ag>
>
> ---
> 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,
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] PATCH V2: fix NULL dereferences / races between task completition and abort
2012-08-15 7:09 ` [Qemu-devel] [PATCH] " Stefan Priebe
2012-08-15 7:10 ` Stefan Priebe - Profihost AG
@ 2012-08-15 8:13 ` ronnie sahlberg
2012-08-15 10:20 ` Kevin Wolf
1 sibling, 1 reply; 7+ messages in thread
From: ronnie sahlberg @ 2012-08-15 8:13 UTC (permalink / raw)
To: Stefan Priebe; +Cc: qemu-devel
Acked-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
On Wed, Aug 15, 2012 at 5:09 PM, Stefan Priebe <s.priebe@profihost.ag> wrote:
> Signed-off-by: Stefan Priebe <s.priebe@profihost.ag>
>
> ---
> 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,
> --
> 1.7.2.5
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] PATCH V2: fix NULL dereferences / races between task completition and abort
2012-08-15 8:13 ` ronnie sahlberg
@ 2012-08-15 10:20 ` Kevin Wolf
0 siblings, 0 replies; 7+ messages in thread
From: Kevin Wolf @ 2012-08-15 10:20 UTC (permalink / raw)
To: ronnie sahlberg; +Cc: qemu-devel, Stefan Priebe
Am 15.08.2012 10:13, schrieb ronnie sahlberg:
> Acked-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
>
>
>
> On Wed, Aug 15, 2012 at 5:09 PM, Stefan Priebe <s.priebe@profihost.ag> wrote:
>> Signed-off-by: Stefan Priebe <s.priebe@profihost.ag>
>>
>> ---
>> 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;
>> }
Fixed the indentation here (which checkpatch unfortunately doesn't seem
to recognise) and applied to the block branch. Thanks.
Kevin
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-08-15 10:21 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-08-15 7:09 [Qemu-devel] PATCH V2: fix NULL dereferences / races between task completition and abort Stefan Priebe
2012-08-15 7:09 ` [Qemu-devel] [PATCH] " Stefan Priebe
2012-08-15 7:10 ` Stefan Priebe - Profihost AG
2012-08-15 8:13 ` ronnie sahlberg
2012-08-15 10:20 ` Kevin Wolf
-- strict thread matches above, loose matches on Subject: below --
2012-08-15 7:05 Stefan Priebe
2012-08-14 20:29 Stefan Priebe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).