Re: [RFC PATCH v5 04/15] KVM: guest_memfd: Track mappability within a struct kvm_gmem_private

[Fuad Tabba <tabba@google.com>]

Hi Gavin,

On Fri, 24 Jan 2025 at 05:32, Gavin Shan <gshan@redhat.com> wrote:
>
> Hi Fuad,
>
> On 1/18/25 2:29 AM, Fuad Tabba wrote:
> > From: Ackerley Tng <ackerleytng@google.com>
> >
> > Track whether guest_memfd memory can be mapped within the inode,
> > since it is property of the guest_memfd's memory contents.
> >
> > The guest_memfd PRIVATE memory attribute is not used for two
> > reasons. First because it reflects the userspace expectation for
> > that memory location, and therefore can be toggled by userspace.
> > The second is, although each guest_memfd file has a 1:1 binding
> > with a KVM instance, the plan is to allow multiple files per
> > inode, e.g. to allow intra-host migration to a new KVM instance,
> > without destroying guest_memfd.
> >
> > Signed-off-by: Ackerley Tng <ackerleytng@google.com>
> > Co-developed-by: Vishal Annapurve <vannapurve@google.com>
> > Signed-off-by: Vishal Annapurve <vannapurve@google.com>
> > Co-developed-by: Fuad Tabba <tabba@google.com>
> > Signed-off-by: Fuad Tabba <tabba@google.com>
> > ---
> >   virt/kvm/guest_memfd.c | 56 ++++++++++++++++++++++++++++++++++++++----
> >   1 file changed, 51 insertions(+), 5 deletions(-)
> >
> > diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> > index 6453658d2650..0a7b6cf8bd8f 100644
> > --- a/virt/kvm/guest_memfd.c
> > +++ b/virt/kvm/guest_memfd.c
> > @@ -18,6 +18,17 @@ struct kvm_gmem {
> >       struct list_head entry;
> >   };
> >
> > +struct kvm_gmem_inode_private {
> > +#ifdef CONFIG_KVM_GMEM_MAPPABLE
> > +     struct xarray mappable_offsets;
> > +#endif
> > +};
> > +
> > +static struct kvm_gmem_inode_private *kvm_gmem_private(struct inode *inode)
> > +{
> > +     return inode->i_mapping->i_private_data;
> > +}
> > +
> >   /**
> >    * folio_file_pfn - like folio_file_page, but return a pfn.
> >    * @folio: The folio which contains this index.
> > @@ -312,8 +323,28 @@ static pgoff_t kvm_gmem_get_index(struct kvm_memory_slot *slot, gfn_t gfn)
> >       return gfn - slot->base_gfn + slot->gmem.pgoff;
> >   }
> >
> > +static void kvm_gmem_evict_inode(struct inode *inode)
> > +{
> > +     struct kvm_gmem_inode_private *private = kvm_gmem_private(inode);
> > +
> > +#ifdef CONFIG_KVM_GMEM_MAPPABLE
> > +     /*
> > +      * .evict_inode can be called before private data is set up if there are
> > +      * issues during inode creation.
> > +      */
> > +     if (private)
> > +             xa_destroy(&private->mappable_offsets);
> > +#endif
> > +
> > +     truncate_inode_pages_final(inode->i_mapping);
> > +
> > +     kfree(private);
> > +     clear_inode(inode);
> > +}
> > +
> >   static const struct super_operations kvm_gmem_super_operations = {
> > -     .statfs         = simple_statfs,
> > +     .statfs         = simple_statfs,
> > +     .evict_inode    = kvm_gmem_evict_inode,
> >   };
> >
>
> As I understood, ->destroy_inode() may be more suitable place where the xarray is
> released. ->evict_inode() usually detach the inode from the existing struct, to make
> it offline. ->destroy_inode() is actually the place where the associated resource
> (memory) is relased.
>
> Another benefit with ->destroy_inode() is we're not concerned to truncate_inode_pages_final()
> and clear_inode().

I see. I'll give this a try.

>
> >   static int kvm_gmem_init_fs_context(struct fs_context *fc)
> > @@ -440,6 +471,7 @@ static struct inode *kvm_gmem_inode_make_secure_inode(const char *name,
> >                                                     loff_t size, u64 flags)
> >   {
> >       const struct qstr qname = QSTR_INIT(name, strlen(name));
> > +     struct kvm_gmem_inode_private *private;
> >       struct inode *inode;
> >       int err;
> >
> > @@ -448,10 +480,19 @@ static struct inode *kvm_gmem_inode_make_secure_inode(const char *name,
> >               return inode;
> >
> >       err = security_inode_init_security_anon(inode, &qname, NULL);
> > -     if (err) {
> > -             iput(inode);
> > -             return ERR_PTR(err);
> > -     }
> > +     if (err)
> > +             goto out;
> > +
> > +     err = -ENOMEM;
> > +     private = kzalloc(sizeof(*private), GFP_KERNEL);
> > +     if (!private)
> > +             goto out;
> > +
> > +#ifdef CONFIG_KVM_GMEM_MAPPABLE
> > +     xa_init(&private->mappable_offsets);
> > +#endif
> > +
> > +     inode->i_mapping->i_private_data = private;
> >
>
> The whole block of code needs to be guarded by CONFIG_KVM_GMEM_MAPPABLE because
> kzalloc(sizeof(...)) is translated to kzalloc(0) when CONFIG_KVM_GMEM_MAPPABLE
> is disabled, and kzalloc() will always fail. It will lead to unusable guest-memfd
> if CONFIG_KVM_GMEM_MAPPABLE is disabled.

Good point, thanks for pointing this out.

Cheers,
/fuad

> >       inode->i_private = (void *)(unsigned long)flags;
> >       inode->i_op = &kvm_gmem_iops;
> > @@ -464,6 +505,11 @@ static struct inode *kvm_gmem_inode_make_secure_inode(const char *name,
> >       WARN_ON_ONCE(!mapping_unevictable(inode->i_mapping));
> >
> >       return inode;
> > +
> > +out:
> > +     iput(inode);
> > +
> > +     return ERR_PTR(err);
> >   }
> >
> >   static struct file *kvm_gmem_inode_create_getfile(void *priv, loff_t size,
>
> Thanks,
> Gavin
>