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

    > 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. ;-)


     \ /
      Last update: 2011-11-10 07:49    [W:0.027 / U:2.012 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site