Messages in this thread |  | | | Date | Thu, 10 Nov 2011 08:46:20 +0200 (EET) | | From | Pekka Enberg <> | | 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
|  |