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

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

[Fam Zheng]

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>
---
 block/iscsi.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/block/iscsi.c b/block/iscsi.c
index 233f462..c154928 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1339,6 +1339,36 @@ 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) {
+        struct scsi_mode_sense *ms;
+
+        task = iscsi_modesense6_sync(iscsilun->iscsi, iscsilun->lun,
+                                      1, SCSI_MODESENSE_PC_CURRENT,
+                                      0x3F,
+                                      0, 255);
+
+        if (task == NULL) {
+            error_setg(errp, "Failed to send MODE_SENSE10 command: %s\n",
+                       iscsi_get_error(iscsilun->iscsi));
+            ret = -EINVAL;
+            goto out;
+        }
+
+        if (task->status != SCSI_STATUS_GOOD) {
+            error_setg(errp, "MODE_SENSE10 failed: %s\n",
+                       iscsi_get_error(iscsi));
+            ret = -EINVAL;
+            goto out;
+        }
+        ms = scsi_datain_unmarshall(task);
+        if (ms->device_specific_parameter & 0x80) {
+            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);
-- 
1.9.3

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

[Paolo Bonzini]

On 10/29/2014 02:13 PM, 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>
> ---
>  block/iscsi.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 233f462..c154928 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -1339,6 +1339,36 @@ 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) {
> +        struct scsi_mode_sense *ms;
> +
> +        task = iscsi_modesense6_sync(iscsilun->iscsi, iscsilun->lun,
> +                                      1, SCSI_MODESENSE_PC_CURRENT,
> +                                      0x3F,
> +                                      0, 255);
> +
> +        if (task == NULL) {
> +            error_setg(errp, "Failed to send MODE_SENSE10 command: %s\n",

This is MODE SENSE(6).  Fixed and applied.

Paolo

> +                       iscsi_get_error(iscsilun->iscsi));
> +            ret = -EINVAL;
> +            goto out;
> +        }
> +
> +        if (task->status != SCSI_STATUS_GOOD) {
> +            error_setg(errp, "MODE_SENSE10 failed: %s\n",
> +                       iscsi_get_error(iscsi));
> +            ret = -EINVAL;
> +            goto out;
> +        }
> +        ms = scsi_datain_unmarshall(task);
> +        if (ms->device_specific_parameter & 0x80) {
> +            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);
> 

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

[Peter Lieven]

Am 29.10.2014 um 14:31 schrieb Paolo Bonzini:
>
> On 10/29/2014 02:13 PM, 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>
>> ---
>>  block/iscsi.c | 30 ++++++++++++++++++++++++++++++
>>  1 file changed, 30 insertions(+)
>>
>> diff --git a/block/iscsi.c b/block/iscsi.c
>> index 233f462..c154928 100644
>> --- a/block/iscsi.c
>> +++ b/block/iscsi.c
>> @@ -1339,6 +1339,36 @@ 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) {
>> +        struct scsi_mode_sense *ms;
>> +
>> +        task = iscsi_modesense6_sync(iscsilun->iscsi, iscsilun->lun,
>> +                                      1, SCSI_MODESENSE_PC_CURRENT,
>> +                                      0x3F,
>> +                                      0, 255);
>> +
>> +        if (task == NULL) {
>> +            error_setg(errp, "Failed to send MODE_SENSE10 command: %s\n",
> This is MODE SENSE(6).  Fixed and applied.
>
> Paolo
>
>> +                       iscsi_get_error(iscsilun->iscsi));
>> +            ret = -EINVAL;
>> +            goto out;
>> +        }
>> +
>> +        if (task->status != SCSI_STATUS_GOOD) {
>> +            error_setg(errp, "MODE_SENSE10 failed: %s\n",
>> +                       iscsi_get_error(iscsi));
>> +            ret = -EINVAL;
>> +            goto out;
>> +        }
>> +        ms = scsi_datain_unmarshall(task);

scsi_datain_unmarshall may fail. You need to check for NULL here.

Peter

>> +        if (ms->device_specific_parameter & 0x80) {
>> +            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);
>>

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

[Paolo Bonzini]

On 10/29/2014 05:18 PM, Peter Lieven wrote:
> Am 29.10.2014 um 14:31 schrieb Paolo Bonzini:
>>
>> On 10/29/2014 02:13 PM, 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>
>>> ---
>>>  block/iscsi.c | 30 ++++++++++++++++++++++++++++++
>>>  1 file changed, 30 insertions(+)
>>>
>>> diff --git a/block/iscsi.c b/block/iscsi.c
>>> index 233f462..c154928 100644
>>> --- a/block/iscsi.c
>>> +++ b/block/iscsi.c
>>> @@ -1339,6 +1339,36 @@ 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) {
>>> +        struct scsi_mode_sense *ms;
>>> +
>>> +        task = iscsi_modesense6_sync(iscsilun->iscsi, iscsilun->lun,
>>> +                                      1, SCSI_MODESENSE_PC_CURRENT,
>>> +                                      0x3F,
>>> +                                      0, 255);
>>> +
>>> +        if (task == NULL) {
>>> +            error_setg(errp, "Failed to send MODE_SENSE10 command: %s\n",
>> This is MODE SENSE(6).  Fixed and applied.
>>
>> Paolo
>>
>>> +                       iscsi_get_error(iscsilun->iscsi));
>>> +            ret = -EINVAL;
>>> +            goto out;
>>> +        }
>>> +
>>> +        if (task->status != SCSI_STATUS_GOOD) {
>>> +            error_setg(errp, "MODE_SENSE10 failed: %s\n",
>>> +                       iscsi_get_error(iscsi));
>>> +            ret = -EINVAL;
>>> +            goto out;
>>> +        }
>>> +        ms = scsi_datain_unmarshall(task);
> 
> scsi_datain_unmarshall may fail. You need to check for NULL here.

Thanks for the remark, I fixed this too.

Paolo

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

[Peter Lieven]

Am 29.10.2014 um 19:28 schrieb Paolo Bonzini <pbonzini@redhat.com>:

> On 10/29/2014 05:18 PM, Peter Lieven wrote:
>> Am 29.10.2014 um 14:31 schrieb Paolo Bonzini:
>>> 
>>> On 10/29/2014 02:13 PM, 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>
>>>> ---
>>>> block/iscsi.c | 30 ++++++++++++++++++++++++++++++
>>>> 1 file changed, 30 insertions(+)
>>>> 
>>>> diff --git a/block/iscsi.c b/block/iscsi.c
>>>> index 233f462..c154928 100644
>>>> --- a/block/iscsi.c
>>>> +++ b/block/iscsi.c
>>>> @@ -1339,6 +1339,36 @@ 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) {
>>>> +        struct scsi_mode_sense *ms;
>>>> +
>>>> +        task = iscsi_modesense6_sync(iscsilun->iscsi, iscsilun->lun,
>>>> +                                      1, SCSI_MODESENSE_PC_CURRENT,
>>>> +                                      0x3F,
>>>> +                                      0, 255);
>>>> +
>>>> +        if (task == NULL) {
>>>> +            error_setg(errp, "Failed to send MODE_SENSE10 command: %s\n",
>>> This is MODE SENSE(6).  Fixed and applied.
>>> 
>>> Paolo
>>> 
>>>> +                       iscsi_get_error(iscsilun->iscsi));
>>>> +            ret = -EINVAL;
>>>> +            goto out;
>>>> +        }
>>>> +
>>>> +        if (task->status != SCSI_STATUS_GOOD) {
>>>> +            error_setg(errp, "MODE_SENSE10 failed: %s\n",
>>>> +                       iscsi_get_error(iscsi));
>>>> +            ret = -EINVAL;
>>>> +            goto out;
>>>> +        }
>>>> +        ms = scsi_datain_unmarshall(task);
>> 
>> scsi_datain_unmarshall may fail. You need to check for NULL here.
> 
> Thanks for the remark, I fixed this too.

Is it in your repo? I can do some testing tomorrow ;-)

Peter

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

[Peter Lieven]

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

On 29.10.2014 19:28, Paolo Bonzini wrote:
> On 10/29/2014 05:18 PM, Peter Lieven wrote:
>> Am 29.10.2014 um 14:31 schrieb Paolo Bonzini:
>>> On 10/29/2014 02:13 PM, 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>
>>>> ---
>>>>   block/iscsi.c | 30 ++++++++++++++++++++++++++++++
>>>>   1 file changed, 30 insertions(+)
>>>>
>>>> diff --git a/block/iscsi.c b/block/iscsi.c
>>>> index 233f462..c154928 100644
>>>> --- a/block/iscsi.c
>>>> +++ b/block/iscsi.c
>>>> @@ -1339,6 +1339,36 @@ 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) {
>>>> +        struct scsi_mode_sense *ms;
>>>> +
>>>> +        task = iscsi_modesense6_sync(iscsilun->iscsi, iscsilun->lun,
>>>> +                                      1, SCSI_MODESENSE_PC_CURRENT,
>>>> +                                      0x3F,
>>>> +                                      0, 255);
>>>> +
>>>> +        if (task == NULL) {
>>>> +            error_setg(errp, "Failed to send MODE_SENSE10 command: %s\n",
>>> This is MODE SENSE(6).  Fixed and applied.
>>>
>>> Paolo
>>>
>>>> +                       iscsi_get_error(iscsilun->iscsi));
>>>> +            ret = -EINVAL;
>>>> +            goto out;
>>>> +        }
>>>> +
>>>> +        if (task->status != SCSI_STATUS_GOOD) {
>>>> +            error_setg(errp, "MODE_SENSE10 failed: %s\n",
>>>> +                       iscsi_get_error(iscsi));
>>>> +            ret = -EINVAL;
>>>> +            goto out;
>>>> +        }
>>>> +        ms = scsi_datain_unmarshall(task);
>> scsi_datain_unmarshall may fail. You need to check for NULL here.
> Thanks for the remark, I fixed this too.
>
> Paolo

I am not 100% happy with this patch as it may break support for some targets.

It seems that MODESENSE is somewhat tricky with some targets. See function  sd_read_write_protect_flag
in <http://lxr.free-electrons.com/ident?i=sd_read_write_protect_flag>drivers/scsi/sd.c of the Linux kernel.

I would not fail if this modesense fails, but just assume write enabled. Maybe drop a warning.

In the command itself:
  0x3F => SCSI_MODEPAGE_RETURN_ALL_PAGES

Can you please post the resulting patch somewhere for review.

Thanks,
Peter

[-- Attachment #2: Type: text/html, Size: 3499 bytes --]

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

[Fam Zheng]

On Thu, 10/30 11:27, Peter Lieven wrote:
> On 29.10.2014 19:28, Paolo Bonzini wrote:
> >On 10/29/2014 05:18 PM, Peter Lieven wrote:
> >>Am 29.10.2014 um 14:31 schrieb Paolo Bonzini:
> >>>On 10/29/2014 02:13 PM, 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>
> >>>>---
> >>>>  block/iscsi.c | 30 ++++++++++++++++++++++++++++++
> >>>>  1 file changed, 30 insertions(+)
> >>>>
> >>>>diff --git a/block/iscsi.c b/block/iscsi.c
> >>>>index 233f462..c154928 100644
> >>>>--- a/block/iscsi.c
> >>>>+++ b/block/iscsi.c
> >>>>@@ -1339,6 +1339,36 @@ 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) {
> >>>>+        struct scsi_mode_sense *ms;
> >>>>+
> >>>>+        task = iscsi_modesense6_sync(iscsilun->iscsi, iscsilun->lun,
> >>>>+                                      1, SCSI_MODESENSE_PC_CURRENT,
> >>>>+                                      0x3F,
> >>>>+                                      0, 255);
> >>>>+
> >>>>+        if (task == NULL) {
> >>>>+            error_setg(errp, "Failed to send MODE_SENSE10 command: %s\n",
> >>>This is MODE SENSE(6).  Fixed and applied.
> >>>
> >>>Paolo
> >>>
> >>>>+                       iscsi_get_error(iscsilun->iscsi));
> >>>>+            ret = -EINVAL;
> >>>>+            goto out;
> >>>>+        }
> >>>>+
> >>>>+        if (task->status != SCSI_STATUS_GOOD) {
> >>>>+            error_setg(errp, "MODE_SENSE10 failed: %s\n",
> >>>>+                       iscsi_get_error(iscsi));
> >>>>+            ret = -EINVAL;
> >>>>+            goto out;
> >>>>+        }
> >>>>+        ms = scsi_datain_unmarshall(task);
> >>scsi_datain_unmarshall may fail. You need to check for NULL here.
> >Thanks for the remark, I fixed this too.
> >
> >Paolo
> 
> I am not 100% happy with this patch as it may break support for some targets.
> 
> It seems that MODESENSE is somewhat tricky with some targets. See function  sd_read_write_protect_flag
> in <http://lxr.free-electrons.com/ident?i=sd_read_write_protect_flag>drivers/scsi/sd.c of the Linux kernel.
> 
> I would not fail if this modesense fails, but just assume write enabled. Maybe drop a warning.

OK.

> 
> In the command itself:
>  0x3F => SCSI_MODEPAGE_RETURN_ALL_PAGES

That requires a newer version of libiscsi than we currently check for.
Seeing drivers/scsi/sd.c used 0x3F, I left this.

> 
> Can you please post the resulting patch somewhere for review.

I'll fix this with the error messsage and unmarshal failure and post another
version.

Fam

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

[Peter Lieven]

On 30.10.2014 11:50, Fam Zheng wrote:
> On Thu, 10/30 11:27, Peter Lieven wrote:
>> On 29.10.2014 19:28, Paolo Bonzini wrote:
>>> On 10/29/2014 05:18 PM, Peter Lieven wrote:
>>>> Am 29.10.2014 um 14:31 schrieb Paolo Bonzini:
>>>>> On 10/29/2014 02:13 PM, 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>
>>>>>> ---
>>>>>>   block/iscsi.c | 30 ++++++++++++++++++++++++++++++
>>>>>>   1 file changed, 30 insertions(+)
>>>>>>
>>>>>> diff --git a/block/iscsi.c b/block/iscsi.c
>>>>>> index 233f462..c154928 100644
>>>>>> --- a/block/iscsi.c
>>>>>> +++ b/block/iscsi.c
>>>>>> @@ -1339,6 +1339,36 @@ 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) {
>>>>>> +        struct scsi_mode_sense *ms;
>>>>>> +
>>>>>> +        task = iscsi_modesense6_sync(iscsilun->iscsi, iscsilun->lun,
>>>>>> +                                      1, SCSI_MODESENSE_PC_CURRENT,
>>>>>> +                                      0x3F,
>>>>>> +                                      0, 255);
>>>>>> +
>>>>>> +        if (task == NULL) {
>>>>>> +            error_setg(errp, "Failed to send MODE_SENSE10 command: %s\n",
>>>>> This is MODE SENSE(6).  Fixed and applied.
>>>>>
>>>>> Paolo
>>>>>
>>>>>> +                       iscsi_get_error(iscsilun->iscsi));
>>>>>> +            ret = -EINVAL;
>>>>>> +            goto out;
>>>>>> +        }
>>>>>> +
>>>>>> +        if (task->status != SCSI_STATUS_GOOD) {
>>>>>> +            error_setg(errp, "MODE_SENSE10 failed: %s\n",
>>>>>> +                       iscsi_get_error(iscsi));
>>>>>> +            ret = -EINVAL;
>>>>>> +            goto out;
>>>>>> +        }
>>>>>> +        ms = scsi_datain_unmarshall(task);
>>>> scsi_datain_unmarshall may fail. You need to check for NULL here.
>>> Thanks for the remark, I fixed this too.
>>>
>>> Paolo
>> I am not 100% happy with this patch as it may break support for some targets.
>>
>> It seems that MODESENSE is somewhat tricky with some targets. See function  sd_read_write_protect_flag
>> in <http://lxr.free-electrons.com/ident?i=sd_read_write_protect_flag>drivers/scsi/sd.c of the Linux kernel.
>>
>> I would not fail if this modesense fails, but just assume write enabled. Maybe drop a warning.
> OK.
>
>> In the command itself:
>>   0x3F => SCSI_MODEPAGE_RETURN_ALL_PAGES
> That requires a newer version of libiscsi than we currently check for.
> Seeing drivers/scsi/sd.c used 0x3F, I left this.

Okay, I wasn't ware. It seems it went in for 1.10.0 and we check for 1.9.0, right?


>
>> Can you please post the resulting patch somewhere for review.
> I'll fix this with the error messsage and unmarshal failure and post another
> version.

Thanks,
Peter