[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Qemu-block] [PATCH v2 3/3] block/iscsi: fix ioctl canc
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [Qemu-block] [PATCH v2 3/3] block/iscsi: fix ioctl cancel use-after-free |
Date: |
Fri, 9 Feb 2018 18:48:19 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 |
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