| Message ID | 20240726235234.228822-3-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: Stop grabbing references to PFNMAP'd pages | expand |
Sean Christopherson <seanjc@google.com> writes: > Disallow copying MTE tags to guest memory while KVM is dirty logging, as > writing guest memory without marking the gfn as dirty in the memslot could > result in userspace failing to migrate the updated page. Ideally (maybe?), > KVM would simply mark the gfn as dirty, but there is no vCPU to work with, > and presumably the only use case for copy MTE tags _to_ the guest is when > restoring state on the target. > > Fixes: f0376edb1ddc ("KVM: arm64: Add ioctl to fetch/store tags in a guest") > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/arm64/kvm/guest.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c > index e1f0ff08836a..962f985977c2 100644 > --- a/arch/arm64/kvm/guest.c > +++ b/arch/arm64/kvm/guest.c > @@ -1045,6 +1045,11 @@ int kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm, > > mutex_lock(&kvm->slots_lock); > > + if (write && atomic_read(&kvm->nr_memslots_dirty_logging)) { > + ret = -EBUSY; > + goto out; > + } > + > is this equivalent to kvm_follow_pfn() with kfp->pin = 1 ? Should all those pin request fail if kvm->nr_memslots_dirty_logging != 0? > while (length > 0) { > kvm_pfn_t pfn = gfn_to_pfn_prot(kvm, gfn, write, NULL); > void *maddr; > -- > 2.46.0.rc1.232.g9752f9e123-goog
On Thu, Aug 01, 2024, Aneesh Kumar K.V wrote: > Sean Christopherson <seanjc@google.com> writes: > > > Disallow copying MTE tags to guest memory while KVM is dirty logging, as > > writing guest memory without marking the gfn as dirty in the memslot could > > result in userspace failing to migrate the updated page. Ideally (maybe?), > > KVM would simply mark the gfn as dirty, but there is no vCPU to work with, > > and presumably the only use case for copy MTE tags _to_ the guest is when > > restoring state on the target. > > > > Fixes: f0376edb1ddc ("KVM: arm64: Add ioctl to fetch/store tags in a guest") > > Signed-off-by: Sean Christopherson <seanjc@google.com> > > --- > > arch/arm64/kvm/guest.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c > > index e1f0ff08836a..962f985977c2 100644 > > --- a/arch/arm64/kvm/guest.c > > +++ b/arch/arm64/kvm/guest.c > > @@ -1045,6 +1045,11 @@ int kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm, > > > > mutex_lock(&kvm->slots_lock); > > > > + if (write && atomic_read(&kvm->nr_memslots_dirty_logging)) { > > + ret = -EBUSY; > > + goto out; > > + } > > + > > > > is this equivalent to kvm_follow_pfn() with kfp->pin = 1 ? No, gfn_to_pfn_prot() == FOLL_GET, kfp->pin == FOLL_PIN. But that's not really relevant. > Should all those pin request fail if kvm->nr_memslots_dirty_logging != 0? No, the conflict with dirty logging is specifically that this code doesn't invoke mark_page_dirty(). And it can't easily do that, because there's no loaded ("running") vCPU, i.e. doing so would trip this WARN: #ifdef CONFIG_HAVE_KVM_DIRTY_RING if (WARN_ON_ONCE(vcpu && vcpu->kvm != kvm)) return; WARN_ON_ONCE(!vcpu && !kvm_arch_allow_write_without_running_vcpu(kvm)); <==== #endif
Sean Christopherson <seanjc@google.com> writes: > On Thu, Aug 01, 2024, Aneesh Kumar K.V wrote: >> Sean Christopherson <seanjc@google.com> writes: >> >> > Disallow copying MTE tags to guest memory while KVM is dirty logging, as >> > writing guest memory without marking the gfn as dirty in the memslot could >> > result in userspace failing to migrate the updated page. Ideally (maybe?), >> > KVM would simply mark the gfn as dirty, but there is no vCPU to work with, >> > and presumably the only use case for copy MTE tags _to_ the guest is when >> > restoring state on the target. >> > >> > Fixes: f0376edb1ddc ("KVM: arm64: Add ioctl to fetch/store tags in a guest") >> > Signed-off-by: Sean Christopherson <seanjc@google.com> >> > --- >> > arch/arm64/kvm/guest.c | 5 +++++ >> > 1 file changed, 5 insertions(+) >> > >> > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c >> > index e1f0ff08836a..962f985977c2 100644 >> > --- a/arch/arm64/kvm/guest.c >> > +++ b/arch/arm64/kvm/guest.c >> > @@ -1045,6 +1045,11 @@ int kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm, >> > >> > mutex_lock(&kvm->slots_lock); >> > >> > + if (write && atomic_read(&kvm->nr_memslots_dirty_logging)) { >> > + ret = -EBUSY; >> > + goto out; >> > + } >> > + >> > >> >> is this equivalent to kvm_follow_pfn() with kfp->pin = 1 ? > > No, gfn_to_pfn_prot() == FOLL_GET, kfp->pin == FOLL_PIN. But that's not really > relevant. > What I meant was, should we consider mte_copy_tags_from_user() as one that update the page contents (even though it is updating tags) and use kvm_follow_pfn() with kfp->pin = 1 instead? Is my understanding correct in that, if we want to look up a pfn/page from gfn with the intent of updating the page contents, we should use kfp->pin == 1? -aneesh
On Mon, Aug 05, 2024, Aneesh Kumar K.V wrote: > Sean Christopherson <seanjc@google.com> writes: > > > On Thu, Aug 01, 2024, Aneesh Kumar K.V wrote: > >> Sean Christopherson <seanjc@google.com> writes: > >> > >> > Disallow copying MTE tags to guest memory while KVM is dirty logging, as > >> > writing guest memory without marking the gfn as dirty in the memslot could > >> > result in userspace failing to migrate the updated page. Ideally (maybe?), > >> > KVM would simply mark the gfn as dirty, but there is no vCPU to work with, > >> > and presumably the only use case for copy MTE tags _to_ the guest is when > >> > restoring state on the target. > >> > > >> > Fixes: f0376edb1ddc ("KVM: arm64: Add ioctl to fetch/store tags in a guest") > >> > Signed-off-by: Sean Christopherson <seanjc@google.com> > >> > --- > >> > arch/arm64/kvm/guest.c | 5 +++++ > >> > 1 file changed, 5 insertions(+) > >> > > >> > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c > >> > index e1f0ff08836a..962f985977c2 100644 > >> > --- a/arch/arm64/kvm/guest.c > >> > +++ b/arch/arm64/kvm/guest.c > >> > @@ -1045,6 +1045,11 @@ int kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm, > >> > > >> > mutex_lock(&kvm->slots_lock); > >> > > >> > + if (write && atomic_read(&kvm->nr_memslots_dirty_logging)) { > >> > + ret = -EBUSY; > >> > + goto out; > >> > + } > >> > + > >> > > >> > >> is this equivalent to kvm_follow_pfn() with kfp->pin = 1 ? > > > > No, gfn_to_pfn_prot() == FOLL_GET, kfp->pin == FOLL_PIN. But that's not really > > relevant. > > What I meant was, should we consider mte_copy_tags_from_user() as one > that update the page contents (even though it is updating tags) and > use kvm_follow_pfn() with kfp->pin = 1 instead? Yes, that's my understanding as well. However, this series is already ludicruosly long, and I don't have the ability to test the affected code, so rather than blindly churn more arch code, I opted to add a FIXME in patch 76 instead. https://lore.kernel.org/all/20240726235234.228822-76-seanjc@google.com
On Fri, Jul 26, 2024 at 04:51:11PM -0700, Sean Christopherson wrote: > Disallow copying MTE tags to guest memory while KVM is dirty logging, as > writing guest memory without marking the gfn as dirty in the memslot could > result in userspace failing to migrate the updated page. Ideally (maybe?), > KVM would simply mark the gfn as dirty, but there is no vCPU to work with, > and presumably the only use case for copy MTE tags _to_ the guest is when > restoring state on the target. > > Fixes: f0376edb1ddc ("KVM: arm64: Add ioctl to fetch/store tags in a guest") > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/arm64/kvm/guest.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c > index e1f0ff08836a..962f985977c2 100644 > --- a/arch/arm64/kvm/guest.c > +++ b/arch/arm64/kvm/guest.c > @@ -1045,6 +1045,11 @@ int kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm, > > mutex_lock(&kvm->slots_lock); > > + if (write && atomic_read(&kvm->nr_memslots_dirty_logging)) { > + ret = -EBUSY; > + goto out; > + } There are ways to actually log the page dirtying but I don't think it's worth it. AFAICT, reading the tags still works and that's what's used during migration (on the VM where dirty tracking takes place). Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
On 07/08/2024 17:21, Catalin Marinas wrote: > On Fri, Jul 26, 2024 at 04:51:11PM -0700, Sean Christopherson wrote: >> Disallow copying MTE tags to guest memory while KVM is dirty logging, as >> writing guest memory without marking the gfn as dirty in the memslot could >> result in userspace failing to migrate the updated page. Ideally (maybe?), >> KVM would simply mark the gfn as dirty, but there is no vCPU to work with, >> and presumably the only use case for copy MTE tags _to_ the guest is when >> restoring state on the target. >> >> Fixes: f0376edb1ddc ("KVM: arm64: Add ioctl to fetch/store tags in a guest") >> Signed-off-by: Sean Christopherson <seanjc@google.com> >> --- >> arch/arm64/kvm/guest.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c >> index e1f0ff08836a..962f985977c2 100644 >> --- a/arch/arm64/kvm/guest.c >> +++ b/arch/arm64/kvm/guest.c >> @@ -1045,6 +1045,11 @@ int kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm, >> >> mutex_lock(&kvm->slots_lock); >> >> + if (write && atomic_read(&kvm->nr_memslots_dirty_logging)) { >> + ret = -EBUSY; >> + goto out; >> + } > > There are ways to actually log the page dirtying but I don't think > it's worth it. AFAICT, reading the tags still works and that's what's > used during migration (on the VM where dirty tracking takes place). > > Reviewed-by: Catalin Marinas <catalin.marinas@arm.com> > Looks sensible to me - my initial thought was "why would a VMM do that?". But it would make sense to actually return a failure rather than letting the VMM shoot itself in the foot. If there's actually a use-case then we could look at making the dirty tracking work, but I'm not convinced there is a good reason. Reviewed-by: Steven Price <steven.price@arm.com> Thanks, Steve
On Fri, 26 Jul 2024 16:51:11 -0700, Sean Christopherson wrote: > Disallow copying MTE tags to guest memory while KVM is dirty logging, as > writing guest memory without marking the gfn as dirty in the memslot could > result in userspace failing to migrate the updated page. Ideally (maybe?), > KVM would simply mark the gfn as dirty, but there is no vCPU to work with, > and presumably the only use case for copy MTE tags _to_ the guest is when > restoring state on the target. > > [...] Applied to next, thanks! [02/84] KVM: arm64: Disallow copying MTE to guest memory while KVM is dirty logging commit: e0b7de4fd18c47ebd47ec0dd1af6503d4071b943 Cheers, M.
diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c index e1f0ff08836a..962f985977c2 100644 --- a/arch/arm64/kvm/guest.c +++ b/arch/arm64/kvm/guest.c @@ -1045,6 +1045,11 @@ int kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm, mutex_lock(&kvm->slots_lock); + if (write && atomic_read(&kvm->nr_memslots_dirty_logging)) { + ret = -EBUSY; + goto out; + } + while (length > 0) { kvm_pfn_t pfn = gfn_to_pfn_prot(kvm, gfn, write, NULL); void *maddr;
Disallow copying MTE tags to guest memory while KVM is dirty logging, as writing guest memory without marking the gfn as dirty in the memslot could result in userspace failing to migrate the updated page. Ideally (maybe?), KVM would simply mark the gfn as dirty, but there is no vCPU to work with, and presumably the only use case for copy MTE tags _to_ the guest is when restoring state on the target. Fixes: f0376edb1ddc ("KVM: arm64: Add ioctl to fetch/store tags in a guest") Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/arm64/kvm/guest.c | 5 +++++ 1 file changed, 5 insertions(+)