[PATCH] lib: Autodetect backing format and specify it explicitly.
In the guestfs_disk_create API we have traditionally allowed you to set backingfile without setting backingformat. The meaning of this is to let qemu autodetect the backing format when opening the overlay disk. However libvirt >= 6.0 refuses to even pass such disks to qemu (see https://bugzilla.redhat.com/show_bug.cgi?id=1798148). For this reason, move the autodetection earlier and make it explicit. We now autodetect the format of the backing disk at the time of creation of the overlay, and set that as the backing format in the overlay disk itself, allowing libvirt to open the disk later. --- lib/create.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/lib/create.c b/lib/create.c index e2a59b88d..e30286c39 100644 --- a/lib/create.c +++ b/lib/create.c @@ -255,6 +255,7 @@ disk_create_qcow2 (guestfs_h *g, const char *filename, int64_t size, const struct guestfs_disk_create_argv *optargs) { const char *backingformat = NULL; + CLEANUP_FREE char *backingformat_free = NULL; const char *preallocation = NULL; const char *compat = NULL; int clustersize = -1; @@ -302,6 +303,18 @@ disk_create_qcow2 (guestfs_h *g, const char *filename, int64_t size, } } + /* With libvirt >= 6.0 the backing format must be specified. */ + if (backingfile != NULL && backingformat == NULL) { + backingformat = backingformat_free = guestfs_disk_format (g, backingfile); + if (!backingformat) + return -1; + if (STREQ (backingformat, "unknown")) { + error (g, _("could not auto-detect the format of the backing file %s"), + backingfile); + return -1; + } + } + /* Assemble the qemu-img command line. */ guestfs_int_cmd_add_arg (cmd, "qemu-img"); guestfs_int_cmd_add_arg (cmd, "create"); -- 2.25.0
On Thu, Feb 06, 2020 at 17:20:19 +0000, Richard W.M. Jones wrote:
In the guestfs_disk_create API we have traditionally allowed you to set backingfile without setting backingformat. The meaning of this is to let qemu autodetect the backing format when opening the overlay disk.
However libvirt >= 6.0 refuses to even pass such disks to qemu (see https://bugzilla.redhat.com/show_bug.cgi?id=1798148).
I thought about this for a while and I think with blockdev we can securely and without data loss allow image detection of one layer and given that it doesn't support backing files (raw) or doesn't have any backing store specified use it with qemu. With such configuration it would basically allow previous semantics. The above rules combined with the fact that we can now ask qemu to refrain from opening backing files allow us to do it securely. The necessity to not have backing image is necessary to prevent any silent corruption of images. The above error will be reported in cases when libvirt can't introspect the image, e.g. when on some unknown network storage. I'll post patches next week hopefully. In such case this patch might not be necessary.
On Thursday, 6 February 2020 18:20:19 CET Richard W.M. Jones wrote:
In the guestfs_disk_create API we have traditionally allowed you to set backingfile without setting backingformat. The meaning of this is to let qemu autodetect the backing format when opening the overlay disk.
However libvirt >= 6.0 refuses to even pass such disks to qemu (see https://bugzilla.redhat.com/show_bug.cgi?id=1798148).
For this reason, move the autodetection earlier and make it explicit. We now autodetect the format of the backing disk at the time of creation of the overlay, and set that as the backing format in the overlay disk itself, allowing libvirt to open the disk later. --- lib/create.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/lib/create.c b/lib/create.c index e2a59b88d..e30286c39 100644 --- a/lib/create.c +++ b/lib/create.c @@ -255,6 +255,7 @@ disk_create_qcow2 (guestfs_h *g, const char *filename, int64_t size, const struct guestfs_disk_create_argv *optargs) { const char *backingformat = NULL; + CLEANUP_FREE char *backingformat_free = NULL; const char *preallocation = NULL; const char *compat = NULL; int clustersize = -1; @@ -302,6 +303,18 @@ disk_create_qcow2 (guestfs_h *g, const char *filename, int64_t size, } }
+ /* With libvirt >= 6.0 the backing format must be specified. */ + if (backingfile != NULL && backingformat == NULL) { + backingformat = backingformat_free = guestfs_disk_format (g, backingfile); + if (!backingformat) + return -1; + if (STREQ (backingformat, "unknown")) { + error (g, _("could not auto-detect the format of the backing file %s"), + backingfile); + return -1; + } + }
I see this patch was pushed, even if it was not reviewed, and in general the problem was more (not totally) on libvirt side. Was it intentional? -- Pino Toscano
On Mon, Mar 09, 2020 at 11:07:20AM +0100, Pino Toscano wrote:
On Thursday, 6 February 2020 18:20:19 CET Richard W.M. Jones wrote:
In the guestfs_disk_create API we have traditionally allowed you to set backingfile without setting backingformat. The meaning of this is to let qemu autodetect the backing format when opening the overlay disk.
However libvirt >= 6.0 refuses to even pass such disks to qemu (see https://bugzilla.redhat.com/show_bug.cgi?id=1798148).
For this reason, move the autodetection earlier and make it explicit. We now autodetect the format of the backing disk at the time of creation of the overlay, and set that as the backing format in the overlay disk itself, allowing libvirt to open the disk later. --- lib/create.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/lib/create.c b/lib/create.c index e2a59b88d..e30286c39 100644 --- a/lib/create.c +++ b/lib/create.c @@ -255,6 +255,7 @@ disk_create_qcow2 (guestfs_h *g, const char *filename, int64_t size, const struct guestfs_disk_create_argv *optargs) { const char *backingformat = NULL; + CLEANUP_FREE char *backingformat_free = NULL; const char *preallocation = NULL; const char *compat = NULL; int clustersize = -1; @@ -302,6 +303,18 @@ disk_create_qcow2 (guestfs_h *g, const char *filename, int64_t size, } }
+ /* With libvirt >= 6.0 the backing format must be specified. */ + if (backingfile != NULL && backingformat == NULL) { + backingformat = backingformat_free = guestfs_disk_format (g, backingfile); + if (!backingformat) + return -1; + if (STREQ (backingformat, "unknown")) { + error (g, _("could not auto-detect the format of the backing file %s"), + backingfile); + return -1; + } + }
I see this patch was pushed, even if it was not reviewed, and in general the problem was more (not totally) on libvirt side.
Was it intentional?
Yes, I pushed it intentionally. Peter has a partial fix on the libvirt side, but it seems to me they are heading in the direction of requiring that the format of backing files in the chain being specified. Since our API specifies that a NULL backing format means the format will be autodetected (but not by what), I moved the detection to here. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
On Monday, 9 March 2020 11:40:46 CET Richard W.M. Jones wrote:
On Mon, Mar 09, 2020 at 11:07:20AM +0100, Pino Toscano wrote:
On Thursday, 6 February 2020 18:20:19 CET Richard W.M. Jones wrote:
In the guestfs_disk_create API we have traditionally allowed you to set backingfile without setting backingformat. The meaning of this is to let qemu autodetect the backing format when opening the overlay disk.
However libvirt >= 6.0 refuses to even pass such disks to qemu (see https://bugzilla.redhat.com/show_bug.cgi?id=1798148).
For this reason, move the autodetection earlier and make it explicit. We now autodetect the format of the backing disk at the time of creation of the overlay, and set that as the backing format in the overlay disk itself, allowing libvirt to open the disk later. --- lib/create.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/lib/create.c b/lib/create.c index e2a59b88d..e30286c39 100644 --- a/lib/create.c +++ b/lib/create.c @@ -255,6 +255,7 @@ disk_create_qcow2 (guestfs_h *g, const char *filename, int64_t size, const struct guestfs_disk_create_argv *optargs) { const char *backingformat = NULL; + CLEANUP_FREE char *backingformat_free = NULL; const char *preallocation = NULL; const char *compat = NULL; int clustersize = -1; @@ -302,6 +303,18 @@ disk_create_qcow2 (guestfs_h *g, const char *filename, int64_t size, } }
+ /* With libvirt >= 6.0 the backing format must be specified. */ + if (backingfile != NULL && backingformat == NULL) { + backingformat = backingformat_free = guestfs_disk_format (g, backingfile); + if (!backingformat) + return -1; + if (STREQ (backingformat, "unknown")) { + error (g, _("could not auto-detect the format of the backing file %s"), + backingfile); + return -1; + } + }
I see this patch was pushed, even if it was not reviewed, and in general the problem was more (not totally) on libvirt side.
Was it intentional?
Yes, I pushed it intentionally. Peter has a partial fix on the libvirt side, but it seems to me they are heading in the direction of requiring that the format of backing files in the chain being specified. Since our API specifies that a NULL backing format means the format will be autodetected (but not by what), I moved the detection to here.
One of the arguments was that using qemu-img to detect the format of an image was unsafe in any case, and thus libvirt does not use it (it has own parsing code). To sum up the situation: - if using qemu-img is safe (with more or less hardening on top), then there is no reason to not do that in libvirt - if using qemu-img is not safe, then I do not see why libguestfs does it while libvirt avoids it Hence, I think this patch in libguestfs is not correct, no matter which POV/course of action we choose. -- Pino Toscano
On Mon, Mar 09, 2020 at 12:07:08PM +0100, Pino Toscano wrote:
On Monday, 9 March 2020 11:40:46 CET Richard W.M. Jones wrote:
On Mon, Mar 09, 2020 at 11:07:20AM +0100, Pino Toscano wrote:
On Thursday, 6 February 2020 18:20:19 CET Richard W.M. Jones wrote:
In the guestfs_disk_create API we have traditionally allowed you to set backingfile without setting backingformat. The meaning of this is to let qemu autodetect the backing format when opening the overlay disk.
However libvirt >= 6.0 refuses to even pass such disks to qemu (see https://bugzilla.redhat.com/show_bug.cgi?id=1798148).
For this reason, move the autodetection earlier and make it explicit. We now autodetect the format of the backing disk at the time of creation of the overlay, and set that as the backing format in the overlay disk itself, allowing libvirt to open the disk later. --- lib/create.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/lib/create.c b/lib/create.c index e2a59b88d..e30286c39 100644 --- a/lib/create.c +++ b/lib/create.c @@ -255,6 +255,7 @@ disk_create_qcow2 (guestfs_h *g, const char *filename, int64_t size, const struct guestfs_disk_create_argv *optargs) { const char *backingformat = NULL; + CLEANUP_FREE char *backingformat_free = NULL; const char *preallocation = NULL; const char *compat = NULL; int clustersize = -1; @@ -302,6 +303,18 @@ disk_create_qcow2 (guestfs_h *g, const char *filename, int64_t size, } }
+ /* With libvirt >= 6.0 the backing format must be specified. */ + if (backingfile != NULL && backingformat == NULL) { + backingformat = backingformat_free = guestfs_disk_format (g, backingfile); + if (!backingformat) + return -1; + if (STREQ (backingformat, "unknown")) { + error (g, _("could not auto-detect the format of the backing file %s"), + backingfile); + return -1; + } + }
I see this patch was pushed, even if it was not reviewed, and in general the problem was more (not totally) on libvirt side.
Was it intentional?
Yes, I pushed it intentionally. Peter has a partial fix on the libvirt side, but it seems to me they are heading in the direction of requiring that the format of backing files in the chain being specified. Since our API specifies that a NULL backing format means the format will be autodetected (but not by what), I moved the detection to here.
One of the arguments was that using qemu-img to detect the format of an image was unsafe in any case, and thus libvirt does not use it (it has own parsing code).
To sum up the situation: - if using qemu-img is safe (with more or less hardening on top), then there is no reason to not do that in libvirt - if using qemu-img is not safe, then I do not see why libguestfs does it while libvirt avoids it Hence, I think this patch in libguestfs is not correct, no matter which POV/course of action we choose.
Well we can certainly revert the patch if you think this is wrong. Dan, any views on this? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
On Mon, Mar 09, 2020 at 11:19:39AM +0000, Richard W.M. Jones wrote:
On Mon, Mar 09, 2020 at 12:07:08PM +0100, Pino Toscano wrote:
On Monday, 9 March 2020 11:40:46 CET Richard W.M. Jones wrote:
On Mon, Mar 09, 2020 at 11:07:20AM +0100, Pino Toscano wrote:
On Thursday, 6 February 2020 18:20:19 CET Richard W.M. Jones wrote:
In the guestfs_disk_create API we have traditionally allowed you to set backingfile without setting backingformat. The meaning of this is to let qemu autodetect the backing format when opening the overlay disk.
However libvirt >= 6.0 refuses to even pass such disks to qemu (see https://bugzilla.redhat.com/show_bug.cgi?id=1798148).
For this reason, move the autodetection earlier and make it explicit. We now autodetect the format of the backing disk at the time of creation of the overlay, and set that as the backing format in the overlay disk itself, allowing libvirt to open the disk later. --- lib/create.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/lib/create.c b/lib/create.c index e2a59b88d..e30286c39 100644 --- a/lib/create.c +++ b/lib/create.c @@ -255,6 +255,7 @@ disk_create_qcow2 (guestfs_h *g, const char *filename, int64_t size, const struct guestfs_disk_create_argv *optargs) { const char *backingformat = NULL; + CLEANUP_FREE char *backingformat_free = NULL; const char *preallocation = NULL; const char *compat = NULL; int clustersize = -1; @@ -302,6 +303,18 @@ disk_create_qcow2 (guestfs_h *g, const char *filename, int64_t size, } }
+ /* With libvirt >= 6.0 the backing format must be specified. */ + if (backingfile != NULL && backingformat == NULL) { + backingformat = backingformat_free = guestfs_disk_format (g, backingfile); + if (!backingformat) + return -1; + if (STREQ (backingformat, "unknown")) { + error (g, _("could not auto-detect the format of the backing file %s"), + backingfile); + return -1; + } + }
I see this patch was pushed, even if it was not reviewed, and in general the problem was more (not totally) on libvirt side.
Was it intentional?
Yes, I pushed it intentionally. Peter has a partial fix on the libvirt side, but it seems to me they are heading in the direction of requiring that the format of backing files in the chain being specified. Since our API specifies that a NULL backing format means the format will be autodetected (but not by what), I moved the detection to here.
One of the arguments was that using qemu-img to detect the format of an image was unsafe in any case, and thus libvirt does not use it (it has own parsing code).
To sum up the situation: - if using qemu-img is safe (with more or less hardening on top), then there is no reason to not do that in libvirt - if using qemu-img is not safe, then I do not see why libguestfs does it while libvirt avoids it Hence, I think this patch in libguestfs is not correct, no matter which POV/course of action we choose.
Well we can certainly revert the patch if you think this is wrong. Dan, any views on this?
Libvirt doesn't do format probing in all cases, but since the 6.1.0 release we do it in enough cases to fix the regressions wrt previous libvirt behaviour. So IIUC, there shouldn't be a need for libguestfs (or other apps) to work around this, except for with 6.0.0 specifically. That one release could be blacklisted, or left as a docs issue. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Mon, Mar 09, 2020 at 11:25:07AM +0000, Daniel P. Berrangé wrote:
Libvirt doesn't do format probing in all cases, but since the 6.1.0 release we do it in enough cases to fix the regressions wrt previous libvirt behaviour. So IIUC, there shouldn't be a need for libguestfs (or other apps) to work around this, except for with 6.0.0 specifically. That one release could be blacklisted, or left as a docs issue.
Alright I'll revert this patch and note the issue in the release notes. Thanks everyone. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
On Mon, Mar 09, 2020 at 12:37:58 +0000, Richard W.M. Jones wrote:
On Mon, Mar 09, 2020 at 11:25:07AM +0000, Daniel P. Berrangé wrote:
Libvirt doesn't do format probing in all cases, but since the 6.1.0 release we do it in enough cases to fix the regressions wrt previous libvirt behaviour. So IIUC, there shouldn't be a need for libguestfs (or other apps) to work around this, except for with 6.0.0 specifically. That one release could be blacklisted, or left as a docs issue.
Alright I'll revert this patch and note the issue in the release notes.
Please mention that both 5.10.0 and 6.0.0 are not suitable.
On Mon, Mar 09, 2020 at 10:40:46 +0000, Richard W.M. Jones wrote:
On Mon, Mar 09, 2020 at 11:07:20AM +0100, Pino Toscano wrote:
On Thursday, 6 February 2020 18:20:19 CET Richard W.M. Jones wrote:
[...]
@@ -302,6 +303,18 @@ disk_create_qcow2 (guestfs_h *g, const char *filename, int64_t size, } }
+ /* With libvirt >= 6.0 the backing format must be specified. */ + if (backingfile != NULL && backingformat == NULL) { + backingformat = backingformat_free = guestfs_disk_format (g, backingfile); + if (!backingformat) + return -1; + if (STREQ (backingformat, "unknown")) { + error (g, _("could not auto-detect the format of the backing file %s"), + backingfile); + return -1; + } + }
I see this patch was pushed, even if it was not reviewed, and in general the problem was more (not totally) on libvirt side.
Was it intentional?
Yes, I pushed it intentionally. Peter has a partial fix on the libvirt side, but it seems to me they are heading in the direction of requiring that the format of backing files in the chain being specified. Since our API specifies that a NULL backing format means the format will be autodetected (but not by what), I moved the detection to here.
Just a note. The reason I actually started re-thinking the issue was that offloading the detection would make users come up with their own and get it wrong. This patch is an example of when the detection is done in a way which allows insecure behaviour. See the 'Important' part here: https://libvirt.org/kbase/backing_chains.html#troubleshooting
participants (4)
-
Daniel P. Berrangé -
Peter Krempa -
Pino Toscano -
Richard W.M. Jones