kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: David Woodhouse <dwmw2@infradead.org>
To: Paolo Bonzini <pbonzini@redhat.com>,
	butt3rflyh4ck <butterflyhuangxx@gmail.com>
Cc: kvm@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>
Subject: Re: [EXTERNAL] There is a null-ptr-deref bug in kvm_dirty_ring_get in virt/kvm/dirty_ring.c
Date: Tue, 16 Nov 2021 16:22:07 +0000	[thread overview]
Message-ID: <a6d8416c50ba86b57f7f193c1ea2f388de90c0bc.camel@infradead.org> (raw)
In-Reply-To: <9eb83cdd-9314-0d1f-0d4b-0cf4432e1e84@redhat.com>

[-- Attachment #1: Type: text/plain, Size: 2131 bytes --]

On Thu, 2021-10-21 at 22:08 +0200, Paolo Bonzini wrote:
> On 18/10/21 19:14, butt3rflyh4ck wrote:
> > {
> > struct kvm_vcpu *vcpu = kvm_get_running_vcpu();  //-------> invoke
> > kvm_get_running_vcpu() to get a vcpu.
> > 
> > WARN_ON_ONCE(vcpu->kvm != kvm); [1]
> > 
> > return &vcpu->dirty_ring;
> > }
> > ```
> > but we had not called KVM_CREATE_VCPU ioctl to create a kvm_vcpu so
> > vcpu is NULL.
> 
> It's not just because there was no call to KVM_CREATE_VCPU; in general
> kvm->dirty_ring_size only works if all writes are associated to a
> specific vCPU, which is not the case for the one of
> kvm_xen_shared_info_init.
> 
> David, what do you think?  Making dirty-page ring buffer incompatible
> with Xen is ugly and I'd rather avoid it; taking the mutex for vcpu 0 is
> not an option because, as the reporter said, you might not have even
> created a vCPU yet when you call KVM_XEN_HVM_SET_ATTR.  The remaining
> option would be just "do not mark the page as dirty if the ring buffer
> is active".  This is feasible because userspace itself has passed the
> shared info gfn; but again, it's ugly...


Sorry for delayed response.

So, from the point of view of Xen guests setting a shinfo page at
runtime, there *is* a vCPU running, and it's made the
XENMAPSPACE_shared_info hypercall to request that the shinfo page be
mapped.

So from KVM's point of view, that vCPU will have exited with
KVM_EXIT_XEN / KVM_EXIT_XEN_HCALL. The VMM is then invoking the
KVM_XEN_HVM_SET_ATTR ioctl from that vCPU's userspace thread, while the
KVM vCPU is idle.

I suppose we could make it a KVM_XEN_VCPU_SET_ATTR instead, and thus
associate it with a particular CPU at least for the initial wallclock
write?

Interrupts get delivered to the shinfo page too, but those will also
have a vCPU associated with them.

On live migration / live update the solution isn't quite so natural;
right now it *is* restored before any vCPUs are created. But if the
kernel made it a KVM_XEN_VCPU_SET_ATTR then the VMM will just have to
restore it as part of restoring the BSP or something. That can work?

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5174 bytes --]

  parent reply	other threads:[~2021-11-16 16:22 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-18 17:14 butt3rflyh4ck
2021-10-21 20:08 ` Paolo Bonzini
2021-10-28  7:42   ` butt3rflyh4ck
2021-11-08  5:11   ` butt3rflyh4ck
2021-11-16 15:41   ` butt3rflyh4ck
2021-11-16 16:22   ` David Woodhouse [this message]
2021-11-16 17:07     ` [EXTERNAL] " David Woodhouse
2021-11-17  9:46   ` Woodhouse, David
2021-11-17 16:49     ` Paolo Bonzini
2021-11-17 18:10       ` Woodhouse, David
2021-11-20 10:16   ` KVM: Warn if mark_page_dirty() is called without an active vCPU David Woodhouse
2021-11-22 17:01     ` Sean Christopherson
2021-11-22 17:52       ` David Woodhouse
2021-11-22 18:49         ` Sean Christopherson
2022-01-13 12:06     ` Christian Borntraeger
2022-01-13 12:14       ` Paolo Bonzini
2022-01-13 12:29         ` [PATCH] KVM: avoid warning on s390 in mark_page_dirty Christian Borntraeger
2022-01-13 12:31           ` David Woodhouse
2022-01-18  8:37           ` Christian Borntraeger
2022-01-18  8:44             ` Paolo Bonzini
2022-01-18  8:53               ` Christian Borntraeger
2022-01-18 11:44                 ` Paolo Bonzini
2022-01-13 12:30         ` KVM: Warn if mark_page_dirty() is called without an active vCPU David Woodhouse
2022-01-13 12:51           ` Christian Borntraeger
2022-01-13 13:22             ` David Woodhouse
2022-01-13 15:09               ` Christian Borntraeger
2022-01-13 14:36           ` Paolo Bonzini

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a6d8416c50ba86b57f7f193c1ea2f388de90c0bc.camel@infradead.org \
    --to=dwmw2@infradead.org \
    --cc=butterflyhuangxx@gmail.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=pbonzini@redhat.com \
    --subject='Re: [EXTERNAL] There is a null-ptr-deref bug in kvm_dirty_ring_get in virt/kvm/dirty_ring.c' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).