From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36408) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XjnLX-0007FX-2t for qemu-devel@nongnu.org; Thu, 30 Oct 2014 06:52:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XjnLR-0000Cb-D2 for qemu-devel@nongnu.org; Thu, 30 Oct 2014 06:52:51 -0400 Received: from mx-v6.kamp.de ([2a02:248:0:51::16]:44212 helo=mx01.kamp.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XjnLR-0000Ak-28 for qemu-devel@nongnu.org; Thu, 30 Oct 2014 06:52:45 -0400 Message-ID: <54521877.6080100@kamp.de> Date: Thu, 30 Oct 2014 11:52:39 +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> <5452127F.3040604@kamp.de> <20141030105013.GB29989@fam-t430.nay.redhat.com> In-Reply-To: <20141030105013.GB29989@fam-t430.nay.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit 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: Fam Zheng Cc: Kevin Wolf , Paolo Bonzini , Ronnie Sahlberg , qemu-devel@nongnu.org, Stefan Hajnoczi 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 >>>>>> --- >>>>>> 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. > 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