On Fri, Feb 09, 2018 at 06:50:06PM +0100, Paolo Bonzini wrote: > On 03/02/2018 07:16, Stefan Hajnoczi wrote: > > iscsi_aio_cancel() does not increment the request's reference count, > > causing a use-after-free when ABORT TASK finishes after the request has > > already completed. > > > > There are some additional issues with iscsi_aio_cancel(): > > 1. Several ABORT TASKs may be sent for the same task if > > iscsi_aio_cancel() is invoked multiple times. It's better to avoid > > this just in case the command identifier is reused. > > 2. The iscsilun->mutex protection is missing in iscsi_aio_cancel(). > > > > Reported-by: Felipe Franciosi <fel...@nutanix.com> > > Signed-off-by: Stefan Hajnoczi <stefa...@redhat.com> > > --- > > block/iscsi.c | 21 ++++++++++++++++++--- > > 1 file changed, 18 insertions(+), 3 deletions(-) > > > > diff --git a/block/iscsi.c b/block/iscsi.c > > index 1cfe1c647c..8140baac15 100644 > > --- a/block/iscsi.c > > +++ b/block/iscsi.c > > @@ -119,6 +119,7 @@ typedef struct IscsiAIOCB { > > #ifdef __linux__ > > sg_io_hdr_t *ioh; > > #endif > > + bool cancelled; > > } IscsiAIOCB; > > > > /* libiscsi uses time_t so its enough to process events every second */ > > @@ -282,6 +283,7 @@ static void iscsi_co_init_iscsitask(IscsiLun *iscsilun, > > struct IscsiTask *iTask) > > }; > > } > > > > +/* Called (via iscsi_service) with QemuMutex held. */ > > static void > > iscsi_abort_task_cb(struct iscsi_context *iscsi, int status, void > > *command_data, > > void *private_data) > > @@ -290,6 +292,7 @@ iscsi_abort_task_cb(struct iscsi_context *iscsi, int > > status, void *command_data, > > > > acb->status = -ECANCELED; > > iscsi_schedule_bh(acb); > > + qemu_aio_unref(acb); /* acquired in iscsi_aio_cancel() */ > > } > > > > static void > > @@ -298,14 +301,25 @@ iscsi_aio_cancel(BlockAIOCB *blockacb) > > IscsiAIOCB *acb = (IscsiAIOCB *)blockacb; > > IscsiLun *iscsilun = acb->iscsilun; > > > > - if (acb->status != -EINPROGRESS) { > > + qemu_mutex_lock(&iscsilun->mutex); > > + > > + /* If it was cancelled or completed already, our work is done here */ > > + if (acb->cancelled || acb->status != -EINPROGRESS) { > > + qemu_mutex_unlock(&iscsilun->mutex); > > return; > > } > > > > + acb->cancelled = true; > > + > > + qemu_aio_ref(acb); /* released in iscsi_abort_task_cb() */ > > + > > /* 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); > > + if (iscsi_task_mgmt_abort_task_async(iscsilun->iscsi, acb->task, > > + iscsi_abort_task_cb, acb) < 0) { > > + qemu_aio_unref(acb); /* since iscsi_abort_task_cb() won't be > > called */ > > + } > > > > + qemu_mutex_unlock(&iscsilun->mutex); > > } > > > > static const AIOCBInfo iscsi_aiocb_info = { > > @@ -1000,6 +1014,7 @@ static BlockAIOCB *iscsi_aio_ioctl(BlockDriverState > > *bs, > > acb->bh = NULL; > > acb->status = -EINPROGRESS; > > acb->ioh = buf; > > + acb->cancelled = false; > > > > if (req != SG_IO) { > > iscsi_ioctl_handle_emulated(acb, req, buf); > > > > BTW, this is okay even without the follow-up, since libiscsi seems not > to obey its contract...
I'm awaiting feedback from Felipe to see if these fixes plus the follow-up have solved the issue. Then I'll send a new revision. Stefan
signature.asc
Description: PGP signature