* [Qemu-devel] [PATCH v2] iscsi: Refuse to open as writable if the LUN is write protected
@ 2014-10-30 11:23 Fam Zheng
2014-10-30 11:43 ` Peter Lieven
0 siblings, 1 reply; 5+ messages in thread
From: Fam Zheng @ 2014-10-30 11:23 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Paolo Bonzini, Peter Lieven, Stefan Hajnoczi,
Ronnie Sahlberg
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) {
+ 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);
--
1.9.3
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v2] iscsi: Refuse to open as writable if the LUN is write protected
2014-10-30 11:23 [Qemu-devel] [PATCH v2] iscsi: Refuse to open as writable if the LUN is write protected Fam Zheng
@ 2014-10-30 11:43 ` Peter Lieven
2014-10-30 12:09 ` Paolo Bonzini
0 siblings, 1 reply; 5+ messages in thread
From: Peter Lieven @ 2014-10-30 11:43 UTC (permalink / raw)
To: Fam Zheng, qemu-devel
Cc: Kevin Wolf, Paolo Bonzini, Stefan Hajnoczi, Ronnie Sahlberg
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
...........................................................
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v2] iscsi: Refuse to open as writable if the LUN is write protected
2014-10-30 11:43 ` Peter Lieven
@ 2014-10-30 12:09 ` Paolo Bonzini
2014-10-30 12:31 ` Fam Zheng
2014-10-30 12:55 ` Peter Lieven
0 siblings, 2 replies; 5+ messages in thread
From: Paolo Bonzini @ 2014-10-30 12:09 UTC (permalink / raw)
To: Peter Lieven, Fam Zheng, qemu-devel
Cc: Kevin Wolf, Stefan Hajnoczi, Ronnie Sahlberg
On 10/30/2014 12:43 PM, Peter Lieven wrote:
> 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.
This is a mix of your and Fam's code. Looks good?
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) {
error_report("Failed to send MODE_SENSE(6) command: %s",
iscsi_get_error(iscsilun->iscsi));
goto out;
}
if (task->status != SCSI_STATUS_GOOD) {
error_report("MODE_SENSE(6) failed: %s",
iscsi_get_error(iscsilun->iscsi));
goto out;
}
ms = scsi_datain_unmarshall(task);
if (!ms) {
error_report("Failed to unmarshall MODE_SENSE(6) data: %s",
iscsi_get_error(iscsilun->iscsi));
goto out;
}
wrprotected = ms->device_specific_parameter & 0x80;
out:
if (task) {
scsi_free_scsi_task(task);
}
return wrprotected;
}
Paolo
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v2] iscsi: Refuse to open as writable if the LUN is write protected
2014-10-30 12:09 ` Paolo Bonzini
@ 2014-10-30 12:31 ` Fam Zheng
2014-10-30 12:55 ` Peter Lieven
1 sibling, 0 replies; 5+ messages in thread
From: Fam Zheng @ 2014-10-30 12:31 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Kevin Wolf, Stefan Hajnoczi, Peter Lieven, qemu-devel,
Ronnie Sahlberg
On Thu, 10/30 13:09, Paolo Bonzini wrote:
>
>
> On 10/30/2014 12:43 PM, Peter Lieven wrote:
> > 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.
>
> This is a mix of your and Fam's code. Looks good?
>
> 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) {
> error_report("Failed to send MODE_SENSE(6) command: %s",
> iscsi_get_error(iscsilun->iscsi));
> goto out;
> }
>
> if (task->status != SCSI_STATUS_GOOD) {
> error_report("MODE_SENSE(6) failed: %s",
> iscsi_get_error(iscsilun->iscsi));
> goto out;
> }
> ms = scsi_datain_unmarshall(task);
> if (!ms) {
> error_report("Failed to unmarshall MODE_SENSE(6) data: %s",
> iscsi_get_error(iscsilun->iscsi));
> goto out;
> }
> wrprotected = ms->device_specific_parameter & 0x80;
>
> out:
> if (task) {
> scsi_free_scsi_task(task);
> }
> return wrprotected;
> }
Looks good. Thanks!
Fam
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v2] iscsi: Refuse to open as writable if the LUN is write protected
2014-10-30 12:09 ` Paolo Bonzini
2014-10-30 12:31 ` Fam Zheng
@ 2014-10-30 12:55 ` Peter Lieven
1 sibling, 0 replies; 5+ messages in thread
From: Peter Lieven @ 2014-10-30 12:55 UTC (permalink / raw)
To: Paolo Bonzini, Fam Zheng, qemu-devel
Cc: Kevin Wolf, Stefan Hajnoczi, Ronnie Sahlberg
On 30.10.2014 13:09, Paolo Bonzini wrote:
>
> On 10/30/2014 12:43 PM, Peter Lieven wrote:
>> 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.
> This is a mix of your and Fam's code. Looks good?
>
> 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) {
> error_report("Failed to send MODE_SENSE(6) command: %s",
> iscsi_get_error(iscsilun->iscsi));
> goto out;
> }
>
> if (task->status != SCSI_STATUS_GOOD) {
> error_report("MODE_SENSE(6) failed: %s",
> iscsi_get_error(iscsilun->iscsi));
> goto out;
> }
> ms = scsi_datain_unmarshall(task);
> if (!ms) {
> error_report("Failed to unmarshall MODE_SENSE(6) data: %s",
> iscsi_get_error(iscsilun->iscsi));
> goto out;
> }
> wrprotected = ms->device_specific_parameter & 0x80;
>
> out:
> if (task) {
> scsi_free_scsi_task(task);
> }
> return wrprotected;
> }
i would add the prefix "iSCSI: " to the error_reports as we
have it for other outputs. (noticed this after writing my mail).
Otherwise looks good.
Reviewed-by: Peter Lieven <pl@kamp.de>
and actually also
Tested-by: Peter Lieven <pl@kamp.de>
Peter
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-10-31 15:52 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-10-30 11:23 [Qemu-devel] [PATCH v2] iscsi: Refuse to open as writable if the LUN is write protected Fam Zheng
2014-10-30 11:43 ` Peter Lieven
2014-10-30 12:09 ` Paolo Bonzini
2014-10-30 12:31 ` Fam Zheng
2014-10-30 12:55 ` Peter Lieven
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).