From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43393) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XIFZb-00067b-80 for qemu-devel@nongnu.org; Fri, 15 Aug 2014 07:21:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XIFZT-000513-2V for qemu-devel@nongnu.org; Fri, 15 Aug 2014 07:21:31 -0400 Received: from mx1.redhat.com ([209.132.183.28]:43514) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XIFZS-00050t-QE for qemu-devel@nongnu.org; Fri, 15 Aug 2014 07:21:22 -0400 From: Markus Armbruster References: <1406900401-19550-1-git-send-email-lkurusa@redhat.com> <20140812132034.GM20490@stefanha-thinkpad.redhat.com> <20140812133542.GA6876@localhost.localdomain> <1643597569.19303034.1408027347194.JavaMail.zimbra@redhat.com> <20140814145733.GA2399@localhost.localdomain> <20140815105519.GC3770@noname.redhat.com> Date: Fri, 15 Aug 2014 13:21:19 +0200 In-Reply-To: <20140815105519.GC3770@noname.redhat.com> (Kevin Wolf's message of "Fri, 15 Aug 2014 12:55:19 +0200") Message-ID: <87ioluhuc0.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH 0/3] vpc: support probing of fixed size images List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: Andrew Jones , Fam Zheng , Stefan Weil , Jeff Cody , Levente Kurusa , QEMU Developers , Stefan Hajnoczi Kevin Wolf writes: > 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. 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. > > 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 think we're doing our users a disservice by sticking to the fatally flawed probing. "Broken by default" is just wrong, and "convenience" is no excuse. I believe we can retain 90% of the convenience without the flaws, by defaulting format based on file meta-data only: name and struct stat. This breaks down for things like QCOW2-formatted logical volumes. But those things are exactly *not* the things casual users need. It also breaks down when users call their QCOW2 images foo.vmdk, but I'd call that a feature :)