From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54961) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xjmwu-0003Tv-62 for qemu-devel@nongnu.org; Thu, 30 Oct 2014 06:27:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xjmwo-0005Tk-Ck for qemu-devel@nongnu.org; Thu, 30 Oct 2014 06:27:24 -0400 Received: from mx-v6.kamp.de ([2a02:248:0:51::16]:43752 helo=mx01.kamp.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xjmwn-0005TO-U4 for qemu-devel@nongnu.org; Thu, 30 Oct 2014 06:27:18 -0400 Message-ID: <5452127F.3040604@kamp.de> Date: Thu, 30 Oct 2014 11:27:11 +0100 From: Peter Lieven MIME-Version: 1.0 References: <1414588414-633-1-git-send-email-famz@redhat.com> <5450EC1A.9020502@redhat.com> <54511367.8010703@kamp.de> <545131B1.6020401@redhat.com> In-Reply-To: <545131B1.6020401@redhat.com> Content-Type: multipart/alternative; boundary="------------090903030208060701020909" Subject: Re: [Qemu-devel] [PATCH] iscsi: Refuse to open as writable if the LUN is write protected List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini , Fam Zheng , qemu-devel@nongnu.org Cc: Kevin Wolf , Stefan Hajnoczi , Ronnie Sahlberg This is a multi-part message in MIME format. --------------090903030208060701020909 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit 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 >>>> --- >>>> 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 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 --------------090903030208060701020909 Content-Type: text/html; charset=windows-1252 Content-Transfer-Encoding: 8bit
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 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 --------------090903030208060701020909--