[Qemu-devel] [PATCH v2 0/3] block/iscsi: fix ioctl cancel use-after-free

[Qemu-devel] [PATCH v2 0/3] block/iscsi: fix ioctl cancel use-after-free

[Stefan Hajnoczi]

v2:
 * It was unnecessary to avoid duplicate iscsi_schedule_bh() calls since this
   function already protects against duplicate calls internally [Stefan]

Patches 1 & 2 are cleanups.

Patch 3 fixes cancellation of ioctls.  Felipe showed me a trace where an acb is
cancelled and then completes twice.  The second time around crashes QEMU.

Compile-tested only.

Felipe: Please let us know if this fixes the issue you are seeing.  Thanks!

Stefan Hajnoczi (3):
  block/iscsi: drop unused IscsiAIOCB->buf field
  block/iscsi: take iscsilun->mutex in iscsi_timed_check_events()
  block/iscsi: fix ioctl cancel use-after-free

 block/iscsi.c | 33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH v2 1/3] block/iscsi: drop unused IscsiAIOCB->buf field

[Stefan Hajnoczi]

The IscsiAIOCB->buf field has not been used since commit
e49ab19fcaa617ad6cdfe1ac401327326b6a2552 ("block/iscsi: bump libiscsi
requirement to 1.9.0").  It used to be a linear buffer for old libiscsi
versions that didn't support scatter-gather.  The minimum libiscsi
version supports scatter-gather so we don't linearize buffers anymore.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/iscsi.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 6a1c53711a..cd0738942c 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -112,7 +112,6 @@ typedef struct IscsiAIOCB {
     QEMUBH *bh;
     IscsiLun *iscsilun;
     struct scsi_task *task;
-    uint8_t *buf;
     int status;
     int64_t sector_num;
     int nb_sectors;
@@ -145,9 +144,6 @@ iscsi_bh_cb(void *p)
 
     qemu_bh_delete(acb->bh);
 
-    g_free(acb->buf);
-    acb->buf = NULL;
-
     acb->common.cb(acb->common.opaque, acb->status);
 
     if (acb->task != NULL) {
@@ -925,9 +921,6 @@ iscsi_aio_ioctl_cb(struct iscsi_context *iscsi, int status,
 {
     IscsiAIOCB *acb = opaque;
 
-    g_free(acb->buf);
-    acb->buf = NULL;
-
     acb->status = 0;
     if (status < 0) {
         error_report("Failed to ioctl(SG_IO) to iSCSI lun. %s",
@@ -1002,7 +995,6 @@ static BlockAIOCB *iscsi_aio_ioctl(BlockDriverState *bs,
     acb->iscsilun = iscsilun;
     acb->bh          = NULL;
     acb->status      = -EINPROGRESS;
-    acb->buf         = NULL;
     acb->ioh         = buf;
 
     if (req != SG_IO) {
-- 
2.14.3

[Qemu-devel] [PATCH v2 2/3] block/iscsi: take iscsilun->mutex in iscsi_timed_check_events()

[Stefan Hajnoczi]

Commit d045c466d9e62b4321fadf586d024d54ddfd8bd4 ("iscsi: do not use
aio_context_acquire/release") introduced iscsilun->mutex but appears to
have overlooked iscsi_timed_check_events() when introducing the mutex.

iscsi_service() and iscsi_set_events() must be called with
iscsilun->mutex held.

iscsi_timed_check_events() is invoked from the AioContext and does not
take the mutex.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/iscsi.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/block/iscsi.c b/block/iscsi.c
index cd0738942c..1cfe1c647c 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -339,6 +339,8 @@ static void iscsi_timed_check_events(void *opaque)
 {
     IscsiLun *iscsilun = opaque;
 
+    qemu_mutex_lock(&iscsilun->mutex);
+
     /* check for timed out requests */
     iscsi_service(iscsilun->iscsi, 0);
 
@@ -351,6 +353,8 @@ static void iscsi_timed_check_events(void *opaque)
      * to return to service once this situation changes. */
     iscsi_set_events(iscsilun);
 
+    qemu_mutex_unlock(&iscsilun->mutex);
+
     timer_mod(iscsilun->event_timer,
               qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + EVENT_INTERVAL);
 }
-- 
2.14.3

[Qemu-devel] [PATCH v2 3/3] block/iscsi: fix ioctl cancel use-after-free

[Stefan Hajnoczi]

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 <felipe@nutanix.com>
Signed-off-by: Stefan Hajnoczi <stefanha@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);
-- 
2.14.3

Re: [Qemu-devel] [Qemu-block] [PATCH v2 3/3] block/iscsi: fix ioctl cancel use-after-free

[Paolo Bonzini]

On 03/02/2018 07:16, Stefan Hajnoczi wrote:
> @@ -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() */

qemu_aio_ref is not thread safe.  I think this is fine however, since
all qemu_aio_ref/unref calls should happen in the I/O thread.

Regarding your follow-up patch:

> 
> -    acb->status = -ECANCELED;
> -    iscsi_schedule_bh(acb);
> +    /* If the command callback hasn't been called yet, drop the task */
> +    if (!acb->bh) {
> +        /* Call iscsi_aio_ioctl_cb() with SCSI_STATUS_CANCELLED */
> +        iscsi_scsi_cancel_task(iscsi, acb->task);
> +    }
> +

SCSI_STATUS_CANCELLED is a libiscsi addition and should not go past
iscsi_aio_ioctl_cb.  So you'd need something like this:

diff --git a/block/iscsi.c b/block/iscsi.c
index 6a1c53711a..ace6ca900f 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -928,6 +928,14 @@ iscsi_aio_ioctl_cb(struct iscsi_context *iscsi,
     g_free(acb->buf);
     acb->buf = NULL;

+    if (status == SCSI_STATUS_CANCELLED) {
+        if (!acb->bh) {
+            acb->status = -ECANCELED);
+            iscsi_schedule_bh(acb);
+        }
+        return;
+    }
+
     acb->status = 0;
     if (status < 0) {
         error_report("Failed to ioctl(SG_IO) to iSCSI lun. %s",

Needless to say, it is really unfortunate that there is no mock iSCSI
server to write tests for. :(

Paolo

Re: [Qemu-devel] [PATCH v2 1/3] block/iscsi: drop unused IscsiAIOCB->buf field

[Paolo Bonzini]

On 03/02/2018 07:16, Stefan Hajnoczi wrote:
> The IscsiAIOCB->buf field has not been used since commit
> e49ab19fcaa617ad6cdfe1ac401327326b6a2552 ("block/iscsi: bump libiscsi
> requirement to 1.9.0").  It used to be a linear buffer for old libiscsi
> versions that didn't support scatter-gather.  The minimum libiscsi
> version supports scatter-gather so we don't linearize buffers anymore.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/iscsi.c | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 6a1c53711a..cd0738942c 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -112,7 +112,6 @@ typedef struct IscsiAIOCB {
>      QEMUBH *bh;
>      IscsiLun *iscsilun;
>      struct scsi_task *task;
> -    uint8_t *buf;
>      int status;
>      int64_t sector_num;
>      int nb_sectors;
> @@ -145,9 +144,6 @@ iscsi_bh_cb(void *p)
>  
>      qemu_bh_delete(acb->bh);
>  
> -    g_free(acb->buf);
> -    acb->buf = NULL;
> -
>      acb->common.cb(acb->common.opaque, acb->status);
>  
>      if (acb->task != NULL) {
> @@ -925,9 +921,6 @@ iscsi_aio_ioctl_cb(struct iscsi_context *iscsi, int status,
>  {
>      IscsiAIOCB *acb = opaque;
>  
> -    g_free(acb->buf);
> -    acb->buf = NULL;
> -
>      acb->status = 0;
>      if (status < 0) {
>          error_report("Failed to ioctl(SG_IO) to iSCSI lun. %s",
> @@ -1002,7 +995,6 @@ static BlockAIOCB *iscsi_aio_ioctl(BlockDriverState *bs,
>      acb->iscsilun = iscsilun;
>      acb->bh          = NULL;
>      acb->status      = -EINPROGRESS;
> -    acb->buf         = NULL;
>      acb->ioh         = buf;
>  
>      if (req != SG_IO) {
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH v2 2/3] block/iscsi: take iscsilun->mutex in iscsi_timed_check_events()

[Paolo Bonzini]

On 03/02/2018 07:16, Stefan Hajnoczi wrote:
> Commit d045c466d9e62b4321fadf586d024d54ddfd8bd4 ("iscsi: do not use
> aio_context_acquire/release") introduced iscsilun->mutex but appears to
> have overlooked iscsi_timed_check_events() when introducing the mutex.
> 
> iscsi_service() and iscsi_set_events() must be called with
> iscsilun->mutex held.
> 
> iscsi_timed_check_events() is invoked from the AioContext and does not
> take the mutex.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

> ---
>  block/iscsi.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index cd0738942c..1cfe1c647c 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -339,6 +339,8 @@ static void iscsi_timed_check_events(void *opaque)
>  {
>      IscsiLun *iscsilun = opaque;
>  
> +    qemu_mutex_lock(&iscsilun->mutex);
> +
>      /* check for timed out requests */
>      iscsi_service(iscsilun->iscsi, 0);
>  
> @@ -351,6 +353,8 @@ static void iscsi_timed_check_events(void *opaque)
>       * to return to service once this situation changes. */
>      iscsi_set_events(iscsilun);
>  
> +    qemu_mutex_unlock(&iscsilun->mutex);
> +
>      timer_mod(iscsilun->event_timer,
>                qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + EVENT_INTERVAL);
>  }
> 

Re: [Qemu-devel] [PATCH v2 3/3] block/iscsi: fix ioctl cancel use-after-free

[Paolo Bonzini]

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 <felipe@nutanix.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@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...

Paolo

Re: [Qemu-devel] [PATCH v2 3/3] block/iscsi: fix ioctl cancel use-after-free

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3676 bytes --]

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 <felipe@nutanix.com>
> > Signed-off-by: Stefan Hajnoczi <stefanha@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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v2 3/3] block/iscsi: fix ioctl cancel use-after-free

[Felipe Franciosi]

> On 3 Feb 2018, at 06:16, Stefan Hajnoczi <stefanha@redhat.com> 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 <felipe@nutanix.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Reviewed-by: Felipe Franciosi <felipe@nutanix.com>
Tested-by: Sreejith Mohanan <sreejit.mohanan@nutanix.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);
> -- 
> 2.14.3
> 

Re: [Qemu-devel] [PATCH v2 0/3] block/iscsi: fix ioctl cancel use-after-free

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1019 bytes --]

On Sat, Feb 03, 2018 at 07:16:18AM +0100, Stefan Hajnoczi wrote:
> v2:
>  * It was unnecessary to avoid duplicate iscsi_schedule_bh() calls since this
>    function already protects against duplicate calls internally [Stefan]
> 
> Patches 1 & 2 are cleanups.
> 
> Patch 3 fixes cancellation of ioctls.  Felipe showed me a trace where an acb is
> cancelled and then completes twice.  The second time around crashes QEMU.
> 
> Compile-tested only.
> 
> Felipe: Please let us know if this fixes the issue you are seeing.  Thanks!
> 
> Stefan Hajnoczi (3):
>   block/iscsi: drop unused IscsiAIOCB->buf field
>   block/iscsi: take iscsilun->mutex in iscsi_timed_check_events()
>   block/iscsi: fix ioctl cancel use-after-free
> 
>  block/iscsi.c | 33 ++++++++++++++++++++++-----------
>  1 file changed, 22 insertions(+), 11 deletions(-)

Thanks for the reviews, Paolo and Felipe.

Paolo: Please merge this, I'll send an additional patch that works
around libiscsi's missing cancellation callback.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v2 0/3] block/iscsi: fix ioctl cancel use-after-free

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 1239 bytes --]

On 15/02/18 11:37, Stefan Hajnoczi wrote:
> On Sat, Feb 03, 2018 at 07:16:18AM +0100, Stefan Hajnoczi wrote:
>> v2:
>>  * It was unnecessary to avoid duplicate iscsi_schedule_bh() calls since this
>>    function already protects against duplicate calls internally [Stefan]
>>
>> Patches 1 & 2 are cleanups.
>>
>> Patch 3 fixes cancellation of ioctls.  Felipe showed me a trace where an acb is
>> cancelled and then completes twice.  The second time around crashes QEMU.
>>
>> Compile-tested only.
>>
>> Felipe: Please let us know if this fixes the issue you are seeing.  Thanks!
>>
>> Stefan Hajnoczi (3):
>>   block/iscsi: drop unused IscsiAIOCB->buf field
>>   block/iscsi: take iscsilun->mutex in iscsi_timed_check_events()
>>   block/iscsi: fix ioctl cancel use-after-free
>>
>>  block/iscsi.c | 33 ++++++++++++++++++++++-----------
>>  1 file changed, 22 insertions(+), 11 deletions(-)
> 
> Thanks for the reviews, Paolo and Felipe.
> 
> Paolo: Please merge this, I'll send an additional patch that works
> around libiscsi's missing cancellation callback.
> 

Queued now for 4.0.  It's only been 9 months...

I also queued "block/iscsi: cancel libiscsi task when ABORT TASK TMF
completes".

Paolo


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]