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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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?


>                      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.


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

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