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

[Paolo Bonzini <pbonzini@redhat.com>]

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