Re: [Qemu-devel] [PATCH v2] iscsi: Refuse to open as writable if the LUN is write protected

[Peter Lieven <pl@kamp.de>]

On 30.10.2014 12:23, Fam Zheng wrote:
> Before, when a write protected iSCSI target is attached as scsi-disk
> with BDRV_O_RDWR, we report it as writable, while in fact all writes
> will fail.
>
> One way to improve this is to report write protect flag as true to
> guest, but a even better way is to refuse using a write protected LUN to
> guest.
>
> Target write protect flag is checked with a mode sense query.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
> v2: Improve error message.
>      Fall back to a warning if mode sense failed.
>      Check unmarshal return value.
> ---
>   block/iscsi.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 46 insertions(+)
>
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 233f462..dcacbca 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -1219,6 +1219,44 @@ static void iscsi_attach_aio_context(BlockDriverState *bs,
>                 qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + NOP_INTERVAL);
>   }
>   
> +static bool iscsi_is_write_protected(IscsiLun *iscsilun)
> +{
> +    struct scsi_task *task;
> +    struct scsi_mode_sense *ms = NULL;
> +
> +    task = iscsi_modesense6_sync(iscsilun->iscsi, iscsilun->lun,
> +            1, SCSI_MODESENSE_PC_CURRENT,
> +            0x3F,
> +            0, 255);
> +
> +    if (task == NULL) {
> +        error_report("Failed to send MODE_SENSE6 command: %s",
> +                     iscsi_get_error(iscsilun->iscsi));
> +        goto out;
> +    }
> +
> +    if (task->status != SCSI_STATUS_GOOD) {
> +        error_report("MODE_SENSE6 failed: %s",
> +                     iscsi_get_error(iscsilun->iscsi));
> +        goto out;
> +    }
> +    ms = scsi_datain_unmarshall(task);
> +    if (!ms) {
> +        error_report("MODE_SENSE6 failed: %s",
> +                     iscsi_get_error(iscsilun->iscsi));
> +        goto out;
> +    }
> +out:
> +    if (task) {
> +        scsi_free_scsi_task(task);
> +    }
> +    if (!ms) {

ms points to freed memory after scsi_free_scsi_task.
furthermore the requests likely fails with task->status != SCSI_STATUS_GOOD
if the modesense implementation is broken etc.

I would rework iscsi_is_write_protected to:

static bool iscsi_is_write_protected(IscsiLun *iscsilun)
{
     struct scsi_task *task;
     struct scsi_mode_sense *ms = NULL;
     bool wrprotected = false;

     task = iscsi_modesense6_sync(iscsilun->iscsi, iscsilun->lun,
             1, SCSI_MODESENSE_PC_CURRENT,
             0x3F,
             0, 255);

     if (task == NULL || task->status != SCSI_STATUS_GOOD) {
         goto fail;
     }
     ms = scsi_datain_unmarshall(task);
     if (!ms) {
         goto fail;
     }
     wrprotected = ms->device_specific_parameter & 0x80;
     goto out;
fail:
     error_report("MODE_SENSE6 failed: %s. Assuming write enabled",
                          iscsi_get_error(iscsilun->iscsi));
out:
     if (task) {
         scsi_free_scsi_task(task);
     }
     return wrprotected;
}

Peter


> +        error_report("Assuming write enabled");
> +        return false;
> +    }
> +    return ms->device_specific_parameter & 0x80;
> +}
> +
>   /*
>    * We support iscsi url's on the form
>    * iscsi://[<username>%<password>@]<host>[:<port>]/<targetname>/<lun>
> @@ -1339,6 +1377,14 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
>       scsi_free_scsi_task(task);
>       task = NULL;
>   
> +    /* Check the write protect flag of the LUN if we want to write */
> +    if ((flags & BDRV_O_RDWR)
> +            && iscsi_is_write_protected(iscsilun)) {
> +        error_setg(errp, "Cannot open a write protected LUN as read-write");
> +        ret = -EPERM;
> +        goto out;
> +    }
> +
>       iscsi_readcapacity_sync(iscsilun, &local_err);
>       if (local_err != NULL) {
>           error_propagate(errp, local_err);


-- 

Mit freundlichen Grüßen

Peter Lieven

...........................................................

   KAMP Netzwerkdienste GmbH
   Vestische Str. 89-91 | 46117 Oberhausen
   Tel: +49 (0) 208.89 402-50 | Fax: +49 (0) 208.89 402-40
   pl@kamp.de | http://www.kamp.de

   Geschäftsführer: Heiner Lante | Michael Lante
   Amtsgericht Duisburg | HRB Nr. 12154
   USt-Id-Nr.: DE 120607556

...........................................................