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:   Thu, 13 Jul 2023 22:46:11 +0000
In-Reply-To: <ZFWli2/H5M8MZRiY@google.com> (message from Sean Christopherson
 on Fri, 5 May 2023 17:55:39 -0700)
Mime-Version: 1.0
Message-ID: <diqzr0pb2zws.fsf@ackerleytng-ctop.c.googlers.com>
Subject: Re: Rename restrictedmem => guardedmem? (was: Re: [PATCH v10 0/9]
 KVM: mm: fd-based approach for supporting KVM)
From:   Ackerley Tng <ackerleytng@google.com>
To:     Sean Christopherson <seanjc@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

Sean Christopherson <seanjc@google.com> writes:

> On Fri, May 05, 2023, Ackerley Tng wrote:
>>
>> Hi Sean,
>>
>> Thanks for implementing this POC!
>>
>> ... snip ...
>>
>
> 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).
>
> ... snip ...
>
> My preference is to make it a VM-scoped ioctl(), if it ends up being a KV=
M ioctl()
> and not a common syscall.  If the file isn't tightly coupled to a single =
VM, then
> punching a hole is further complicated by needing to deal with invalidati=
ng multiple
> regions that are bound to different @kvm instances.  It's not super compl=
ex, but
> AFAICT having the ioctl() be system-scoped doesn't add value, e.g. I don'=
t think
> having one VM own the memory will complicate even if/when we get to the p=
oint where
> VMs can share "private" memory, and the gmem code would still need to dea=
l with
> grabbing a module reference.

I=E2=80=99d like to follow up on this discussion about a guest_mem file
outliving the VM and whether to have a VM-scoped ioctl or a KVM ioctl.

Here's a POC of delayed binding of a guest_mem file to a memslot, where
the guest_mem file outlives the VM [1].

I also hope to raise some points before we do the first integration of
guest_mem patches!


A use case for guest_mem inodes outliving the VM is when the host VMM
needs to be upgraded. The guest_mem inode is passed between two VMs on
the same host machine and all memory associated with the inode needs to
be retained.

To support the above use case, binding of memslots is delayed until
first use, so that the following inode passing flow can be used:

1. Source (old version of host VMM) process passes guest_mem inodes to
   destination (new version of host VMM) process via unix sockets.
2. Destination process initializes memslots identical to source process.
3. Destination process invokes ioctl to migrate guest_mem inode over to
   destination process by unbinding all memslots from the source VM and
   binding them to the destination VM. (The kvm pointer is updated in
   this process)

Without delayed binding, step 2 will fail since initialization of
memslots would check and find that the kvm pointer in the guest_mem
inode points to the kvm in the source process.


These two patches contain the meat of the changes required to support
delayed binding:

https://github.com/googleprodkernel/linux-cc/commit/93b31a006ef2e4dbe1ef0ec=
5d2534ca30f3bf60c
https://github.com/googleprodkernel/linux-cc/commit/dd5ac5e53f14a1ef9915c9c=
1e4cc1006a40b49df

Some things to highlight for the approach set out in these two patches:

1. Previously, closing the guest_mem file in userspace=C2=A0is taken to mea=
n
   that all associated memory is to be removed and cleared. With these
   two patches, each memslot also holds a reference to the file (and
   therefore inode) and so even if the host VMM closes the fd, the VM
   will be able to continue to function.

   This is desirable to userspace since closing the file should not be
   interpreted as a command to clear memory. This is aligned with the
   way tmpfs=C2=A0files are used with KVM before guest_mem: when the file i=
s
   closed in userspace, the memory contents are still mapped and can
   still be used by the VM. fallocate(PUNCH_HOLE) is how userspace
   should command memory to be removed, just like munmap() would be used
   to remove memory from use by KVM.

2. Creating a guest mem file no longer depends on a specific VM and
   hence the guest_mem creation ioctl can be a system ioctl instead of a
   VM specific ioctl. This will also address Chao's concern at [3].


I also separated cleaning up files vs inodes in
https://github.com/googleprodkernel/linux-cc/commit/0f5aa18910c515141e57e05=
c4cc791022047a242,
which I believe is more aligned with how files and inodes are cleaned up
in FSes. This alignment makes it easier to extend gmem=C2=A0to hugetlb, for
one. While working on this, I was also wondering if we should perhaps be
storing the inode pointer in slot->gmem instead of the file pointer? The
memory is associated with an inode->mapping rather than the file. Are we
binding to a userspace handle on the inode (store file pointer), or are
we really referencing the inode (store inode pointer)?

The branch [1] doesn't handle the bug Sean previously mentioned at [2]:
Need to take a reference on the KVM module, so that even if guest_mem
files are not bound to any VM, the KVM module cannot be unloaded. If the
KVM module can be unloaded while guest_mem files are open, then
userspace may be able to crash the kernel by invoking guest_mem
functions that had been unloaded together with KVM.


[1] https://github.com/googleprodkernel/linux-cc/tree/gmem-delayed-binding
[2] https://lore.kernel.org/lkml/ZFWli2%2FH5M8MZRiY@google.com/
[3] https://lore.kernel.org/lkml/20230509124428.GA217130@chaop.bj.intel.com=
/