Re: [PATCH v5 3/7] qcow: Tolerate backing_fmt=, but warn on backing_fmt=raw

[Eric Blake]

On 5/5/20 2:35 AM, Kevin Wolf wrote:
> Am 03.04.2020 um 19:58 hat Eric Blake geschrieben:
> > qcow has no space in the metadata to store a backing format, and there
> > are existing qcow images backed both by raw or by other formats
> > (usually qcow) images, reliant on probing to tell the difference.
> > While we don't recommend the creation of new qcow images (as qcow2 is
> > hands-down better), we can at least insist that if the user does
> > request a specific format without using -u, then it must be non-raw
> > (as a raw backing file that gets inadvertently edited into some other
> > format can form a security hole); if the user does not request a
> > specific format or lies when using -u, then the status quo of probing
> > for the backing format remains intact (although an upcoming patch will
> > warn when omitting a format request).  Thus, when this series is
> > complete, the only way to use a backing file for qcow without
> > triggering a warning is when using -F if the backing file is non-raw
> > to begin with.  Note that this is only for QemuOpts usage; there is no
> > change to the QAPI to allow a format through -blockdev.
> >
> > Add a new iotest 290 just for qcow, to demonstrate the new warning.
> >
> > Signed-off-by: Eric Blake <address@hidden>
>
> Somehow this feels backwards. Not specifying the backing file format at
> all isn't any safer than explicitly specifying raw.
>
> If there is a difference at all, I would say that explicitly specifying
> raw means that the user is aware what they are doing. So we would have
> more reason to warn against raw images if the backing format isn't
> specified at all because then the user might not be aware that they are
> using a backing file that probes as raw.

Prior to this patch, -F does not work with qcow.  And even with this 
patch, we still cannot store the explicit value of -F in the qcow file. 
Anything that does not use -F must continue to work for now (although it 
may now warn, and in fact must warn if we deprecate it), while anything 
explicit is free to fail (since it failed already), but could also be 
made to work (if letting it work is nicer than making it fail, and where 
"work" may still include a warning, although it's pointless to have 
something brand new that works but is deprecated out of the box).  So 
the following is my summary of the two options we can choose between:

Option 1, qcow backed by raw is more common than qcow backed by other, 
so we want:
raw <- qcow, no -F: work without warning (but if backing file is edited, 
a future probe seeing non-raw would break image)
raw <- qcow, with -F: work without warning (but if backing file is 
edited, a future probe seeing non-raw would break image)
other <- qcow, no -F: works but issues a warning (but backing file will 
always probe correctly)
other <- qcow, with -F: fails (we cannot honor the user's explicit 
request, because we would still have to probe)

Option 2, qcow backed by other is more common than qcow backed by raw, 
so we want:
raw <- qcow, no -F: works but issues a warning (using a raw backing file 
without explicit buy-in is risky)
raw <- qcow, with -F: works but issues a warning (explicit buy-in will 
still require subsequent probing, and a backing file could change which 
would break image)
other <- qcow, no -F: works without warning
other <- qcow, with -F: works without warning (later probing will still 
see non-raw)

It looks like you are leaning more towards option 1, while my patch 
leaned more towards option 2.  Anyone else want to chime in with an 
opinion on which is safer vs. easier?


> > @@ -953,6 +954,13 @@ static int coroutine_fn qcow_co_create_opts(BlockDriver 
> > *drv,
> >       };
> >
> >       /* Parse options and convert legacy syntax */
> > +    backing_fmt = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FMT);
> > +    if (backing_fmt && !strcmp(backing_fmt, "raw")) {
> > +        error_setg(errp, "qcow cannot store backing format; an explicit "
> > +                   "backing format of raw is unsafe");
>
> Does this message tell that an implicit backing format of raw is safe?

If we go with option 2, are we trying to deprecate ALL use of raw as a 
backing file to qcow, regardless of whether the user was explicit about 
it?  If we go with option 1, then we are instead deprecating any use of 
non-raw as a backing file to qcow.

At the end of the day, we are trying to discourage users from creating 
new qcow files in the first place; qcow2 is much better.  We still have 
to read existing qcow images with backing files, but maybe we want:

Option 3:
completely deprecate qcow images with backing files, as there is no safe 
way to do things favoring either raw (option 1) or non-raw (option 2), 
and therefore accept -F solely for convenience with the rest of the 
series, but always issue a warning regardless of whether -F was present.

> > +        ret = -EINVAL;
> > +        goto fail;
> > +    }
>
> The commit message promises a warning. This is not a warning, but a hard
> error.

Once we decide which behavior we want, I'll make sure the commit message 
matches the behavior.  Remember, for implicit images, we are starting a 
deprecation clock; for explicit images, it previously failed, so 
continuing to fail is still viable.


> > +echo
> > +echo "== qcow backed by raw =="
> > +
> > +rm "$TEST_IMG.base"
> > +truncate --size=$size "$TEST_IMG.base"
> > +_make_test_img -b "$TEST_IMG.base"
> > +_img_info
> > +_make_test_img -b "$TEST_IMG.base" -F raw
> > +_img_info
>
> This test doesn't tell the difference between a warning and an error. In
> both cases, the image would look the same: Either because it was
> successfully created or because the old version is still there.

Indeed, I can make the test more robust.  But more importantly, we need 
consensus on _what_ we want behavior to be, and write the test along 
those lines.

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org