On Thu, Feb 8, 2018 at 2:14 PM, Paolo Bonzini <pbonz...@redhat.com> wrote:
> On 08/02/2018 15:08, Stefan Hajnoczi wrote:
>> Now on to libiscsi:
>>
>> The iscsi_task_mgmt_async() API documentation says:
>>
>> * abort_task will also cancel the scsi task. The callback for the
>> scsi task will be invoked with
>> * SCSI_STATUS_CANCELLED
>>
>> I see that the ABORT TASK TMF response invokes the user's
>> iscsi_task_mgmt_async() callback but not the command callback. I'm
>> not sure how the command callback is invoked with
>> SCSI_STATUS_CANCELLED unless libiscsi is relying on the target to send
>> that response.
>>
>> Is libiscsi honoring its iscsi_task_mgmt_async() contract?
>
> No, and QEMU is assuming the "wrong" behavior:
>
> static void
> iscsi_abort_task_cb(struct iscsi_context *iscsi, int status, void
> *command_data,
> void *private_data)
> {
> IscsiAIOCB *acb = private_data;
>
> acb->status = -ECANCELED;
> iscsi_schedule_bh(acb);
> }
FWIW My recent use-after-free fix series is still pending review:
https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg00570.html
It changes the code and fixes a QEMU bug, but it doesn't address the
iscsi_task_mgmt_async() issue we're discussing now.
I think users *can* work around the iscsi_task_mgmt_async() issue as follows:
If the command callback has not been invoked yet when the ABORT TASK
TMF callback is invoked, call iscsi_scsi_cancel_task() so that
libiscsi invokes the command callback with SCSI_STATUS_CANCELLED and
frees the PDU.
Here is how that looks in QEMU:
diff --git a/block/iscsi.c b/block/iscsi.c
index 8140baac15..7b4b43dc55 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -290,8 +290,12 @@ iscsi_abort_task_cb(struct iscsi_context *iscsi,
int status, void *command_data,
{
IscsiAIOCB *acb = private_data;
- 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);
+ }
+
qemu_aio_unref(acb); /* acquired in iscsi_aio_cancel() */
}
