Re: [RFC/GIT PULL] Linux KVM tool for v3.2

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux