Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 15 May 2023 16:46:51 -0700
In-Reply-To: <ZFWli2/H5M8MZRiY@google.com>
Mime-Version: 1.0
References: <ZEM5Zq8oo+xnApW9@google.com> <diqz8re2ftzb.fsf@ackerleytng-ctop.c.googlers.com>
 <ZFWli2/H5M8MZRiY@google.com>
Message-ID: <ZGLEa2j4wi0t4xLI@google.com>
Subject: Re: Rename restrictedmem => guardedmem? (was: Re: [PATCH v10 0/9]
 KVM: mm: fd-based approach for supporting KVM)
From:   Sean Christopherson <seanjc@google.com>
To:     Ackerley Tng <ackerleytng@google.com>
Cc:     david@redhat.com, chao.p.peng@linux.intel.com, pbonzini@redhat.com,
        vkuznets@redhat.com, jmattson@google.com, joro@8bytes.org,
        mail@maciej.szmigiero.name, vbabka@suse.cz, vannapurve@google.com,
        yu.c.zhang@linux.intel.com, kirill.shutemov@linux.intel.com,
        dhildenb@redhat.com, qperret@google.com, tabba@google.com,
        michael.roth@amd.com, wei.w.wang@intel.com, rppt@kernel.org,
        liam.merwick@oracle.com, isaku.yamahata@gmail.com,
        jarkko@kernel.org, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org, hughd@google.com, brauner@kernel.org
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, May 05, 2023, Sean Christopherson wrote:
> On Fri, May 05, 2023, Ackerley Tng wrote:
> > One issue I=E2=80=99ve found so far is that the pointer to kvm (gmem->k=
vm) is
> > not cleaned up, and hence it is possible to crash the host kernel in th=
e
> > following way
> >=20
> > 1. Create a KVM VM
> > 2. Create a guest mem fd on that VM
> > 3. Create a memslot with the guest mem fd (hence binding the fd to the
> >    VM)
> > 4. Close/destroy the KVM VM
> > 5. Call fallocate(PUNCH_HOLE) on the guest mem fd, which uses gmem->kvm
> >    when it tries to do invalidation.
> >=20
> > I then tried to clean up the gmem->kvm pointer during unbinding when th=
e
> > KVM VM is destroyed.
> >=20
> > That works, but then I realized there=E2=80=99s a simpler way to use th=
e pointer
> > after freeing:
> >=20
> > 1. Create a KVM VM
> > 2. Create a guest mem fd on that VM
> > 3. Close/destroy the KVM VM
> > 4. Call fallocate(PUNCH_HOLE) on the guest mem fd, which uses gmem->kvm
> >    when it tries to do invalidation.
> >=20
> > Perhaps binding should mean setting the gmem->kvm pointer in addition t=
o
> > gmem->bindings. This makes binding and unbinding symmetric and avoids
> > the use-after-frees described above.
>=20
> Hrm, that would work, though it's a bit convoluted, e.g. would require de=
tecting
> when the last binding is being removed.  A similar (also ugly) solution w=
ould be
> to nullify gmem->kvm when KVM dies.
>=20
> I don't love either approach idea because it means a file created in the =
context
> of a VM can outlive the VM itself, and then userspace ends up with a file=
 descriptor
> that it can't do anything with except close().  I doubt that matters in p=
ractice
> though, e.g. when the VM dies, all memory can be freed so that the file e=
nds up
> being little more than a shell.  And if we go that route, there's no need=
 to grab
> a reference to the file during bind, KVM can just grab a longterm referen=
ce when
> the file is initially created and then drop it when KVM dies (and nullifi=
es gmem->kvm).
>=20
> Blech, another wart is that I believe gmem would need to do __module_get(=
) during
> file creation to prevent kvm.ko from being unloaded after the last VM die=
s.  Ah,
> but that'd also be true if we went with a system-scoped KVM ioctl(), so I=
 suppose
> it's not _that_ ugly.
>=20
> Exchanging references (at binding or at creation) doesn't work, because t=
hat
> creates a circular dependency, i.e. gmem and KVM would pin each other.=20
>=20
> A "proper" refcounting approach, where the file pins KVM and not vice ver=
sa, gets
> nasty because of how KVM's memslots work.  The least awful approach I can=
 think of
> would be to delete the associated memslot(s) when the file is released, p=
ossibly
> via deferred work to avoid deadlock issues.  Not the prettiest thing ever=
 and in
> some ways that'd yield an even worse ABI.

Circling back to this.  Pending testing, the "proper" refcounting approach =
seems
to be the way to go.  KVM's existing memslots actually work this way, e.g. =
if
userspace does munmap() on an active memslot, KVM zaps any PTEs but the mem=
slot
stays alive.  A similar approach can be taken here, the only wrinkle is tha=
t the
association between gmem and the memslot is stronger than between VMAs and =
memslots,
specifically KVM binds the file and not simply the file descriptor.  This i=
s
necessary because not binding to an exact file would let userspace install =
a
different file at the file descriptor.

That's solvable without having to delete memslots though, e.g. by protectin=
g the
file pointer in the memslot with RCU and directly bumping the refcount in t=
he two
places where KVM needs to get at gmem (the file) via the memslot (unbind an=
d
faulting in a page).  E.g.

  static struct file *kvm_gmem_get_file(struct kvm_memory_slot *slot)
  {
	struct file *file;

	rcu_read_lock();

	file =3D rcu_dereference(slot->gmem.file);
	if (file && !get_file_rcu(file))
		file =3D NULL;
	rcu_read_unlock();

	return file;
  }

The gotcha is that ->release could race with memslot deletion, as kvm_gmem_=
unbind()
won't be able to differentiate between "file was deleted" and "file is curr=
ently
being deleted".  That's easy enough to deal with though, kvm_gmem_release()=
 can
take slots_lock to prevent the memslot from going away when nullifying and
invalidating ranges for the memslot.

> Side topic, there's a second bug (and probably more lurking): kvm_swap_ac=
tive_memslots()'s
> call to synchronize_srcu_expedited() is done _before_ the call to kvm_gme=
m_unbind(),
> i.e. doesn't wait for readers in kvm_gmem_invalidate_begin() to go away. =
 The easy
> solution for that one is to add another synchronize_srcu_expedited() afte=
r unbinding.

There's a bug here, but not the one I pointed out.  Acquiring kvm->srcu doe=
sn't
provide any protection, the binding already has a pointer to the memslot, i=
.e.
isn't doing an SRCU-protected lookup in the memslots.  The actual protectio=
n is
provided by the filemap invalidate lock, which prevents unbinding a memslot=
 until
all invalidations complete, i.e. acquiring kvm->srcu in the punch hole path=
 is
completely unnecessary.