Re: [Xen-devel] [RFC] libxl: relax readonly check introduced by XSA-142 fix

[Jim Fehlig <jfehlig@xxxxxxxx>]

Stefano Stabellini wrote:
> On Wed, 11 Nov 2015, Jim Fehlig wrote:
> 
>> Hi All,
>>
>> Apologies for only noticing the fix for XSA-142 as it starting flowing to our
>> various downstreams. The current fix seems like quite a big hammer IMO. qemu
>> doesn't support readonly IDE/SATA disks
>>
>> # /usr/lib/xen/bin/qemu-system-i386 -drive
>> file=/tmp/disk.raw,if=ide,media=disk,format=raw,readonly=on
>> qemu-system-i386: Can't use a read-only drive
>>
>> But it does support readonly SCSI disks
>>
>> # /usr/lib/xen/bin/qemu-system-i386 -drive
>> file=/tmp/disk.raw,if=scsi,media=disk,format=raw,readonly=on
>> [ok]
>>
>> Inside a guest using such a disk, the SCSI kernel driver sees write protect 
>> on
>>
>> [   7.339232] sd 2:0:1:0: [sdb] Write Protect is on
>>
>> Also, PV drivers support readonly, but the patch rejects such configuration 
>> even
>> when PV drivers (vdev=xvd*) have been explicitly specified and creation of an
>> emulated twin is skipped.
>>
>> The attached follow-up loosens the restriction to reject readonly when 
>> creating
>> and emulated IDE disk, but allows it when the backend is known to support 
>> readonly.
>>
>> Regards,
>> Jim
> 
> Hi Jim,
> 
> thanks for the patch, I think is good.  Just one question below:
> 
> 
>> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
>> index 9c9eaa3..ccfc827 100644
>> --- a/tools/libxl/libxl_dm.c
>> +++ b/tools/libxl/libxl_dm.c
>> @@ -1152,12 +1152,6 @@ static int 
>> libxl__build_device_model_args_new(libxl__gc *gc,
>>                          (gc, 
>> "file=%s,if=ide,index=%d,readonly=%s,media=cdrom,format=%s,cache=writeback,id=ide-%i",
>>                           disks[i].pdev_path, disk, disks[i].readwrite ? 
>> "off" : "on", format, dev_number);
>>              } else {
>> -                if (!disks[i].readwrite) {
>> -                    LOG(ERROR,
>> -                        "qemu-xen doesn't support read-only disk drivers");
>> -                    return ERROR_INVAL;
>> -                }
>> -
>>                  if (disks[i].format == LIBXL_DISK_FORMAT_EMPTY) {
>>                      LOG(WARN, "cannot support"" empty disk format for %s",
>>                          disks[i].vdev);
>> @@ -1185,29 +1179,34 @@ static int 
>> libxl__build_device_model_args_new(libxl__gc *gc,
>>                   * For other disks we translate devices 0..3 into
>>                   * hd[a-d] and ignore the rest.
>>                   */
>> -                if (strncmp(disks[i].vdev, "sd", 2) == 0)
>> +                if (strncmp(disks[i].vdev, "sd", 2) == 0) {
>>                      drive = libxl__sprintf
>> -                        (gc, 
>> "file=%s,if=scsi,bus=0,unit=%d,format=%s,cache=writeback",
>> -                         pdev_path, disk, format);
>> -                else if (strncmp(disks[i].vdev, "xvd", 3) == 0)
>> +                        (gc, 
>> "file=%s,if=scsi,bus=0,unit=%d,format=%s,readonly=%s,cache=writeback",
>> +                         pdev_path, disk, format, disks[i].readwrite ? 
>> "off" : "on");
>> +                } else if (strncmp(disks[i].vdev, "xvd", 3) == 0) {
>>                      /*
>>                       * Do not add any emulated disk when PV disk are
>>                       * explicitly asked for.
>>                       */
>>                      continue;
>> -                else if (disk < 6 && b_info->u.hvm.hdtype == 
>> LIBXL_HDTYPE_AHCI) {
>> +                } else if (disk < 6 && b_info->u.hvm.hdtype == 
>> LIBXL_HDTYPE_AHCI) {
>>                      flexarray_vappend(dm_args, "-drive",
>> -                        
>> GCSPRINTF("file=%s,if=none,id=ahcidisk-%d,format=%s,cache=writeback",
>> -                        pdev_path, disk, format),
>> +                        
>> GCSPRINTF("file=%s,if=none,id=ahcidisk-%d,format=%s,readonly=%s,cache=writeback",
>> +                                  pdev_path, disk, format, 
>> disks[i].readwrite ? "off" : "on"),
>>                          "-device", 
>> GCSPRINTF("ide-hd,bus=ahci0.%d,unit=0,drive=ahcidisk-%d",
>>                          disk, disk), NULL);
> 
> The commit message doesn't say anything about AHCI. Are AHCI disks
> actually emulated correctly by QEMU with readonly=on?

I just double checked, and good thing since AHCI + readonly is another rejected
combination

/usr/lib/xen/bin/qemu-system-i386 -device ahci,id=ahci0 \
 -drive file=/tmp/disk.raw,if=none,id=ahcidisk-0,format=raw,readonly=on \
 -device ide-hd,bus=ahci0.0,unit=0,drive=ahcidisk-0
qemu-system-i386: -device ide-hd,bus=ahci0.0,unit=0,drive=ahcidisk-0: Can't use
a read-only drive

So IDE/SATA/AHCI are all incompatible with readonly=on. I'll fix this and ammend
the commit message in V2.

> 
> 
>>                      continue;
>> -                } else if (disk < 4)
>> +                } else if (disk < 4) {
>> +                    if (!disks[i].readwrite) {
>> +                        LOG(ERROR, "qemu-xen doesn't support read-only IDE 
>> disk drivers");
>> +                        return ERROR_INVAL;
>> +                    }
>>                      drive = libxl__sprintf
>>                          (gc, 
>> "file=%s,if=ide,index=%d,media=disk,format=%s,cache=writeback",
>>                           pdev_path, disk, format);
>> -                else
>> +                } else {
>>                      continue; /* Do not emulate this disk */
>> +                }
> 
> I don't think that libxl CODING_STYLE requires brackets for one statement
> blocks. But I won't enforce it.

I can remove the unneeded brackets if desired. That habit comes from working on
projects where all if/else blocks are required to have brackets when any one of
the blocks requires brackets.

Regards,
Jim

> 
> 
>>              }
>>  
>>              flexarray_append(dm_args, "-drive");
>> -- 
>> 1.8.0.1
>>
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel