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

[Fuad Tabba <tabba@google.com>]

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.

> > <snip>
> >
> > +
> > +/*
> > + * Callback function for __folio_put(), i.e., called when all references by the
> > + * host to the folio have been dropped. This allows gmem to transition the state
> > + * of the folio to mappable by the guest, and allows the hypervisor to continue
> > + * transitioning its state to private, since the host cannot attempt to access
> > + * it anymore.
> > + */
> > +void kvm_gmem_handle_folio_put(struct folio *folio)
> > +{
> > +     struct xarray *mappable_offsets;
> > +     struct inode *inode;
> > +     pgoff_t index;
> > +     void *xval;
> > +
> > +     inode = folio->mapping->host;
>
> IIUC this will be a NULL pointer dereference if the folio had been
> removed from the filemap, either through truncation or if the
> guest_memfd file got closed.

Ack.

> > +     index = folio->index;
>
> And if removed from the filemap folio->index is probably invalid.

Ack and thanks again,
/fuad

> > +     mappable_offsets = &kvm_gmem_private(inode)->mappable_offsets;
> > +     xval = xa_mk_value(KVM_GMEM_GUEST_MAPPABLE);
> > +
> > +     filemap_invalidate_lock(inode->i_mapping);
> > +     __kvm_gmem_restore_pending_folio(folio);
> > +     WARN_ON_ONCE(xa_err(xa_store(mappable_offsets, index, xval, GFP_KERNEL)));
> > +     filemap_invalidate_unlock(inode->i_mapping);
> > +}
> > +
> >  static bool gmem_is_mappable(struct inode *inode, pgoff_t pgoff)
> >  {
> >       struct xarray *mappable_offsets = &kvm_gmem_private(inode)->mappable_offsets;