Re: [RFC PATCH v5 06/15] KVM: guest_memfd: Handle final folio_put() of guestmem pages

[Ackerley Tng <ackerleytng@google.com>]

Vishal Annapurve <vannapurve@google.com> writes:

> On Thu, Jan 23, 2025 at 1:51 AM Fuad Tabba <tabba@google.com> wrote:
>>
>> On Wed, 22 Jan 2025 at 22:16, Ackerley Tng <ackerleytng@google.com> wrote:
>> >
>> > Fuad Tabba <tabba@google.com> writes:
>> >
>> > Hey Fuad, I'm still working on verifying all this but for now this is
>> > one issue. I think this can be fixed by checking if the folio->mapping
>> > is NULL. If it's NULL, then the folio has been disassociated from the
>> > inode, and during the dissociation (removal from filemap), the
>> > mappability can also either
>> >
>> > 1. Be unset so that the default mappability can be set up based on
>> >    GUEST_MEMFD_FLAG_INIT_MAPPABLE, or
>> > 2. Be directly restored based on GUEST_MEMFD_FLAG_INIT_MAPPABLE
>>
>> Thanks for pointing this out. I hadn't considered this case. I'll fix
>> in the respin.
>>
>
> Can the below scenario cause trouble?
> 1) Userspace converts a certain range of guest memfd as shared and
> grabs some refcounts on shared memory pages through existing kernel
> exposed mechanisms.
> 2) Userspace converts the same range to private which would cause the
> corresponding mappability attributes to be *MAPPABILITY_NONE.
> 3) Userspace truncates the range which will remove the page from pagecache.
> 4) Userspace does the fallocate again, leading to a new page getting
> allocated without freeing the older page which is still refcounted
> (step 1).
>
> Effectively this could allow userspace to keep allocating multiple
> pages for the same guest_memfd range.

I'm still verifying this but for now here's the flow Vishal described in
greater detail:

+ guest_memfd starts without GUEST_MEMFD_FLAG_INIT_MAPPABLE
    + All new pages will start with mappability = GUEST
+ guest uses a page
    + Get new page
    + Add page to filemap
+ guest converts page to shared
    + Mappability is now ALL
+ host uses page
+ host takes transient refcounts on page
    + Refcount on the page is now (a) filemap's refcount (b) vma's refcount
      (c) transient refcount
+ guest converts page to private
    + Page is unmapped
        + Refcount on the page is now (a) filemap's refcount (b) transient
          refcount
    + Since refcount is elevated, the mappabilities are left as NONE
    + Filemap's refcounts are removed from the page
        + Refcount on the page is now (a) transient refcount
+ host punches hole to deallocate page
    + Since mappability was NONE, restore filemap's refcount
        + Refcount on the page is now (a) transient refcount (b) filemap's
          refcount
    + Mappabilities are reset to GUEST for truncated range
    + Folio is removed from filemap
        + Refcount on the page is now (a) transient refcount
    + Callback remains registered so that when the transient refcounts are
      dropped, cleanup can happen - this is where merging will happen
      with 1G page support
+ host fallocate()s in the same address range
    + will get a new page

Though the host does manage to get a new page while the old one stays
around, I think this is working as intended, since the transient
refcounts are truly holding the old folio around. When the transient
refcounts go away, the old folio will still get cleaned up (with 1G page
support: merged and returned) to as expected. The new page will also be
freed at some point later.

If the userspace program decides to keep taking transient refcounts to hold
pages around, then the userspace program is truly leaking memory and it
shouldn't be guest_memfd's bug.