Re: [PATCH] KVM: s390: vsie: fix race during shadow creation

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

 





Am 20.12.23 um 11:39 schrieb David Hildenbrand:
On 20.12.23 08:34, Christian Borntraeger wrote:
Right now it is possible to see gmap->private being zero in
kvm_s390_vsie_gmap_notifier resulting in a crash.  This is due to the
fact that we add gmap->private == kvm after creation:

static int acquire_gmap_shadow(struct kvm_vcpu *vcpu,
                                struct vsie_page *vsie_page)
{
[...]
         gmap = gmap_shadow(vcpu->arch.gmap, asce, edat);
         if (IS_ERR(gmap))
                 return PTR_ERR(gmap);
         gmap->private = vcpu->kvm;

Instead of tracking kvm in the shadow gmap, simply use the parent one.

Cc: David Hildenbrand <david@xxxxxxxxxx>
Signed-off-by: Christian Borntraeger <borntraeger@xxxxxxxxxxxxx>
---
  arch/s390/kvm/vsie.c | 6 ++++--
  1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
index 8207a892bbe2..6b06d8ec41b5 100644
--- a/arch/s390/kvm/vsie.c
+++ b/arch/s390/kvm/vsie.c
@@ -579,14 +579,17 @@ static int shadow_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
  void kvm_s390_vsie_gmap_notifier(struct gmap *gmap, unsigned long start,
                   unsigned long end)
  {
-    struct kvm *kvm = gmap->private;
      struct vsie_page *cur;
      unsigned long prefix;
      struct page *page;
+    struct kvm *kvm;
      int i;
      if (!gmap_is_shadow(gmap))
          return;
+
+    kvm = gmap->parent->private;
+
      /*
       * Only new shadow blocks are added to the list during runtime,
       * therefore we can safely reference them all the time.
@@ -1220,7 +1223,6 @@ static int acquire_gmap_shadow(struct kvm_vcpu *vcpu,
      gmap = gmap_shadow(vcpu->arch.gmap, asce, edat);
      if (IS_ERR(gmap))
          return PTR_ERR(gmap);
-    gmap->private = vcpu->kvm;
      vcpu->kvm->stat.gmap_shadow_create++;
      WRITE_ONCE(vsie_page->gmap, gmap);
      return 0;

Why not let gmap_shadow handle it? Simply clone the parent private field.

diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
index 6f96b5a71c63..e083fade7a5d 100644
--- a/arch/s390/mm/gmap.c
+++ b/arch/s390/mm/gmap.c
@@ -1691,6 +1691,7 @@ struct gmap *gmap_shadow(struct gmap *parent, unsigned long asce,
                 return ERR_PTR(-ENOMEM);
         new->mm = parent->mm;
         new->parent = gmap_get(parent);
+       new->private = patent->private;
         new->orig_asce = asce;
         new->edat_level = edat_level;
         new->initialized = false;

Or am I missing something?

That would work as well. I discussed several alternatives with Janosch.
The only thing that bothers me is that the owner should define private. So an
alternative would be to have a parameter for gmap_shadow. On the other hand I
like the simplicity of this patch. (we need to get rid of the 2nd assignment
in acquire_gmap_shadow to make it complete.

So I can spin a v2 with this variant if Janosch is ok with it as well.





[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Kernel Development]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Info]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Linux Media]     [Device Mapper]

  Powered by Linux