Am 15.08.2014 um 14:14 hat Jeff Cody geschrieben: > On Fri, Aug 15, 2014 at 12:55:19PM +0200, Kevin Wolf wrote: > > Am 14.08.2014 um 16:57 hat Jeff Cody geschrieben: > > > On Thu, Aug 14, 2014 at 10:42:27AM -0400, Levente Kurusa wrote: > > > > On Tuesday, 12 August, 2014 3:35:42 PM, Jeff Cody wrote: > > > > > On Tue, Aug 12, 2014 at 02:20:34PM +0100, Stefan Hajnoczi wrote: > > > > > > On Fri, Aug 01, 2014 at 03:39:58PM +0200, Levente Kurusa wrote: > > > > > > > Fixed size VPC images do not have a footer, hence the current > > > > > > > probe > > > > > > > function will fail and QEMU will fall back to the raw_bsd driver, > > > > > > > which > > > > > > > is > > > > > > > not the correct behaviour. The specification of the format says > > > > > > > that > > > > > > > fixed > > > > > > > size images have a footer as the last 512 bytes of the file. The > > > > > > > footer > > > > > > > is > > > > > > > exactly the same as the header would be in the case of dynamically > > > > > > > growing > > > > > > > images. > > > > > > > > > > > > > > For this, we need to read the last 512 bytes of the image, > > > > > > > however the > > > > > > > current mechanics predominantly read the first 2048 bytes and > > > > > > > pass that > > > > > > > as a buffer to the probe functions. Solve this by passing the > > > > > > > BlockDriverState to the probe functions, hence giving them a > > > > > > > chance to > > > > > > > read > > > > > > > the extra bytes they might need. > > > > > > > > > > > > I hesitate to add patches that extend image format probing. For the > > > > > > past few years we have always recommended that image files should > > > > > > not be > > > > > > probed. > > > > > > > > > > > > Image probing is prone to security issues because a malicious guest > > > > > > can > > > > > > modify a raw or vpc image by putting another image format header at > > > > > > sector 0. The next time QEMU opens the image it will detect a > > > > > > different > > > > > > format. One evil trick is to refer to a file on the host file > > > > > > system as > > > > > > the backing file, now you can read any file that the QEMU process > > > > > > has > > > > > > access to. > > > > > > > > Yea, good point. The current state of probing is actually quite bad, > > > > just take a look at dmg_probe in block/dmg.c :-( > > > > > > > > > > > > > > > > Probing also complicates live migration. The source host still has > > > > > > the > > > > > > image file open and may write to it. The destination host shouldn't > > > > > > even read from the image file before handover to avoid file cache > > > > > > coherency issues. > > > > > > > > > > > > Probing is broken. It shouldn't be used. We shouldn't extend it > > > > > > (especially by adding more I/Os). > > > > > > > > Even though, my series would have only added one extra I/O in the case > > > > of failing VPC images, I have to admit you are right. > > > > > > > > > > > > > > For 2.2, maybe we should limit probing to only certain operations > > > > > (e.g. > > > > > qemu-img info) - or perhaps just remove the capability altogether, or > > > > > at least start phasing it out with a warning message that automatic > > > > > format detection is deprecated and may be unsafe. > > > > > > > > > > > > > Considering the fact that most open functions already check the magic > > > > numbers, and they do a lot better/safer job at it, we could just swap > > > > the probe functions with the open ones and just insert an fprintf > > > > when format is not specified doing what Jeff suggested. > > > > > > > > Any objections to this? > > > > > > > > (This would also solve the VPC-fixed-size bug, since vpc_open already > > > > checks the footer if the header is not found) > > > > > > > > > > I was proposing actually going a bit further than this, and not > > > allowing automatic format detection at all, with an exception for > > > 'qemu-img info'. In the interim, until that is in place and it is > > > removed, warn with a deprecation message. > > > > No, we can't do this. It would immediately destroy -hda and similar > > convenience options and make the command line really hard to use even > > for simple cases. > > > > You are right, we would need a way to specify the image format for the > convenience options (and remove a lot of the convenience). Perhaps > that is a show-stopper right away.
I think it's very close to a show-stopper anyway. But it wouldn't hurt to allow something like -cdrom test.iso,format=raw (or any other -drive options), that already saves some typing compared to -drive. > The other big area of impact is image files that have metadata > specifying a backing file, but not the backing file format. For > instance, this usage still probes when opening 'foo.qcow2': > qemu-img create -f qcow2 foo.qcow2 1G > qemu-img create -f qcow2 bar.qcow2 -b foo.qcow2 > > qemu-kvm -drive file=bar.qcow2,format=qcow2 Ouch, you're right. If the above wasn't a show-stopper, this one is. Breaking all images with backing files so that you need to fix up whole backing file chains with rebase -u is not an option. > As an aside - I've heard us developers say multiple places that we > really recommend against image probing, but our command line options > and user documentation don't reflect that - at least not strongly. > (For qemu-img, the documentation even says image format is "guessed > automatically in most cases"). > > > I usually call qemu manually and I specify format > > basically _never_, because it would like double the length of my command > > line (okay, not quite, but my command lines are usually very short) and > > I know what I'm doing and I'm running trusted guests. > > > > Yes, I do the same, and it is pretty convenient. This change would > be somewhat of a pita. > > > Plus, there are probably many scripts out there that rely on it. > > > > A more reasonable approach would be to just forbid probing raw and > > raw-like formats like VHD fixed (the rest should be safe), but I think > > the impact of this would still be too bad. > > > > I made some patches to add in a deprecation message, and an additional > option so 'qemu-img info' could bypass that message. > > Things immediately noticed: > > * qemu-io didn't have a way to specify image format easily, so that > needed to be added. > * qemu-img didn't have a way to specify image format for some options; > that needed added as well. No matter if we actually deprecate anything or not, these changes are definitely valuable. > * pretty much every qemu io test needed to be modified, to explicitly > specify image format. I'm about 25% through those. > > And of course, convenience options like -hda spit out the deprecation > warning - which I think is probably a good thing. Here is what I made > it say: > > fprintf(stderr, "Format autodetection is deprecated and may be " > "removed in future versions. Image format autodetection " > "is not reliable; some image formats (e.g. raw) may " > "masquerade as other image formats. This could lead to " > "system data loss or leaks.\n"); > > > If we think doing this is a good thing, I'll continue modifying the > qemu-iotests. Otherwise, I'll drop it. I don't think this radical way works. We can choose Markus's suggestion of using the file name to guess the format. I don't really like it much, but it seems like a fair compromise that doesn't hurt usability as much. If we don't want this, we can approach the problem from a different angle: The problem is not probing per se, but that images probed as raw can be written to by guests in a way that the next time they are probed as something else. What if we let the raw driver know that it was probed and then it enables a check that returns -EIO for any write on the first 2k if that write would make the image look like a different format? Kevin