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 <address@hidden>
> > > > > > ---
> > > > > >   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