Date:	Thu, 10 Nov 2011 08:46:20 +0200 (EET)
From:	Pekka Enberg <penberg@...nel.org>
To:	Anthony Liguori <anthony@...emonkey.ws>
cc:	Pekka Enberg <penberg@...helsinki.fi>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Avi Kivity <avi@...hat.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Ingo Molnar <mingo@...e.hu>, linux-kernel@...r.kernel.org,
	kvm@...r.kernel.org, Christoph Hellwig <hch@....de>
Subject: Re: [RFC/GIT PULL] Linux KVM tool for v3.2

Hi Anthony,

> On 11/04/2011 03:38 AM, Pekka Enberg wrote:
>> Hi Linus,
>> 
>> Please consider pulling the latest KVM tool tree from:
>> 
>> git://git.kernel.org/pub/scm/linux/kernel/git/penberg/linux.git 
>> kvmtool/for-linus
>> 
>
> [snip]
>
>> tools/kvm/virtio/net.c | 423 ++++++++
>> tools/kvm/virtio/pci.c | 319 ++++++
>> tools/kvm/virtio/rng.c | 185 ++++
>> 186 files changed, 19071 insertions(+), 179 deletions(-)

On Wed, 9 Nov 2011, Anthony Liguori wrote:
> So let's assume for a moment that a tool like this should live in the kernel. 
> What's disturbing about a PULL request like this is the lack of reviewability 
> of it and the lack of any real review from people that understand what's 
> going on in this code base.
>
> There are no Acked-by's by people that really understand what the code is 
> doing or that have domain expertise in filesystems and networking.
>
> There are major functionality short comings in this code base, data 
> corruptors, and CVEs.  I'm not saying that the kvm-tool developers are bad 
> developers, but the code is not at the appropriate quality level for the 
> kernel.  It just looks pretty on the surface to people that are used to the 
> kernel coding style.
>
> To highlight a few of the issues:
>
> 1) The RTC emulation is limited to emulating CMOS and only the few fields 
> used to store the date and time.  If code is added to arch/x86 that tries to 
> make use of a CMOS field for something useful, kvm-tool is going to fall 
> over.
>
> None of the register A/B/C logic is implemented and none of the timer logic 
> is implemented.  I imagine this requires kernel command line hackery to keep 
> the kernel from throwing up.

The "fake it until you make it" design principle is actually something 
Ingo suggested early on and has been a really important factor in getting 
us to where we are right now.

Not that I disagree with you. I think we should definitely clean up 
our hardware emulation code.

> If a kernel change that works on bare metal but breaks kvm-tool because 
> kvm-tool is incomplete is committed, is that a regression that requires 
> reverting the change in arch/x86?

If it's the KVM tool being silly, obviously not.

> 2) The qcow2 code is a filesystem implemented in userspace.  Image formats 
> are file systems.  It really should be reviewed by the filesystem 
> maintainers. There is absolutely no attempt made to synchronize the metadata 
> during write operations which means that you do not have crash consistency of 
> the meta data.
>
> If you experience a power failure or kvm-tool crashs, your image will get 
> corrupted.  I highly doubt a file system would ever be merged into Linux that 
> was this naive about data integrity.

The QCOW2 is lagging behind because we lost the main developer. It's 
forced as read-only for the issues you mention. If you think it's a merge 
blocker, we can drop it completely from the tree until the issues are 
sorted out.

I personally don't see the issue of having it as a read-only filesystem.

> 3) The block probing code replicates a well known CVE from three years 
> ago[1]. Using kvm-tool, a malicious guest could write the qcow2 signature to 
> the zero sector and use that to attack the host.

We don't support QCOW2 snapshots so I don't see how the "arbitrary file" 
thing can happen.

It's pretty sad though that we're replicating a known security issue :-/

> [1] http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2008-2004

> I found these three issues in the course of about 30 seconds of looking 
> through the kvm-tool code.  I'm sure if other people with expertise in these 
> areas looked through the code, they would find a lot more issues.  I'm sure I 
> could find many, many more issues.

Thanks for the review!

Would you be interested in spending another 30 seconds to find out 
some more issue? :-)

> This is really the problem with the tools/kvm approach.  It circumvents the 
> normal review process in the kernel because the kernel maintainer structure 
> is not equipped to properly review userspace code in tools.  This is a tool 
> with data integrity and security implications.  It is not a pretty printing 
> routine or a test case.
>
> While I think it's a neat and potentially useful project, I think long before 
> we get to the point where we discuss merging it into the kernel, the code 
> quality has to improve considerably.

It's a problem, sure. I think we have a decent track record in fixing up 
issues raised on kvm@. We've probably even fixed most of the issues you 
and Avi pointed out very early on because lets face it, you were right and 
I was wrong about quite a few things. ;-)

 			Pekka
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists