lkml.org 
[lkml]   [2011]   [Nov]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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


\
 
 \ /
  Last update: 2011-11-10 07:49    [from the cache]
©2003-2011 Jasper Spaans