From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:45857)
	by lists.gnu.org with esmtp (Exim 4.71) (envelope-from <pl@kamp.de>)
	id 1Xjo8p-0007sP-8L
	for qemu-devel@nongnu.org; Thu, 30 Oct 2014 07:43:53 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <pl@kamp.de>) id 1Xjo8j-0008Iu-II
	for qemu-devel@nongnu.org; Thu, 30 Oct 2014 07:43:47 -0400
Received: from mx-v6.kamp.de ([2a02:248:0:51::16]:50809 helo=mx01.kamp.de)
	by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from <pl@kamp.de>)
	id 1Xjo8j-0008IL-8e
	for qemu-devel@nongnu.org; Thu, 30 Oct 2014 07:43:41 -0400
Message-ID: <54522467.9060008@kamp.de>
Date: Thu, 30 Oct 2014 12:43:35 +0100
From: Peter Lieven <pl@kamp.de>
MIME-Version: 1.0
References: <1414668226-11463-1-git-send-email-famz@redhat.com>
In-Reply-To: <1414668226-11463-1-git-send-email-famz@redhat.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 8bit
Subject: Re: [Qemu-devel] [PATCH v2] iscsi: Refuse to open as writable if
 the LUN is write protected
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Fam Zheng <famz@redhat.com>, qemu-devel@nongnu.org
Cc: Kevin Wolf <kwolf@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>, Ronnie Sahlberg <ronniesahlberg@gmail.com>

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

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