| Message ID | 1345014594-6020-2-git-send-email-s.priebe@profihost.ag |
|---|---|
| State | New |
| Headers | show |
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,
>
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 > >
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
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,
Signed-off-by: Stefan Priebe <s.priebe@profihost.ag> --- block/iscsi.c | 55 +++++++++++++++++++++++-------------------------------- 1 files changed, 23 insertions(+), 32 deletions(-)