Message-ID: <c558354c-0103-42b7-aa29-741147c9330e@amd.com>
Date: Wed, 13 Aug 2025 11:07:25 +0530
From: "Garg, Shivank" <shivankg@....com>
To: Ackerley Tng <ackerleytng@...gle.com>,
 David Hildenbrand <david@...hat.com>, seanjc@...gle.com, vbabka@...e.cz,
 willy@...radead.org, akpm@...ux-foundation.org, shuah@...nel.org,
 pbonzini@...hat.com, brauner@...nel.org, viro@...iv.linux.org.uk
Cc: paul@...l-moore.com, jmorris@...ei.org, serge@...lyn.com, pvorel@...e.cz,
 bfoster@...hat.com, tabba@...gle.com, vannapurve@...gle.com,
 chao.gao@...el.com, bharata@....com, nikunj@....com, michael.day@....com,
 shdhiman@....com, yan.y.zhao@...el.com, Neeraj.Upadhyay@....com,
 thomas.lendacky@....com, michael.roth@....com, aik@....com, jgg@...dia.com,
 kalyazin@...zon.com, peterx@...hat.com, jack@...e.cz, rppt@...nel.org,
 hch@...radead.org, cgzones@...glemail.com, ira.weiny@...el.com,
 rientjes@...gle.com, roypat@...zon.co.uk, ziy@...dia.com,
 matthew.brost@...el.com, joshua.hahnjy@...il.com, rakie.kim@...com,
 byungchul@...com, gourry@...rry.net, kent.overstreet@...ux.dev,
 ying.huang@...ux.alibaba.com, apopple@...dia.com, chao.p.peng@...el.com,
 amit@...radead.org, ddutile@...hat.com, dan.j.williams@...el.com,
 ashish.kalra@....com, gshan@...hat.com, jgowans@...zon.com,
 pankaj.gupta@....com, papaluri@....com, yuzhao@...gle.com,
 suzuki.poulose@....com, quic_eberman@...cinc.com,
 aneeshkumar.kizhakeveetil@....com, linux-fsdevel@...r.kernel.org,
 linux-mm@...ck.org, linux-kernel@...r.kernel.org,
 linux-security-module@...r.kernel.org, kvm@...r.kernel.org,
 linux-kselftest@...r.kernel.org, linux-coco@...ts.linux.dev
Subject: Re: [PATCH RFC V10 4/7] KVM: guest_memfd: Use guest mem inodes
 instead of anonymous inodes



On 8/12/2025 2:53 AM, Ackerley Tng wrote:
> David Hildenbrand <david@...hat.com> writes:
> 
>> On 11.08.25 11:06, Shivank Garg wrote:
>>> From: Ackerley Tng <ackerleytng@...gle.com>
>>>
>>> [...snip...]
>>>
>>> +static struct file *kvm_gmem_inode_create_getfile(void *priv, loff_t size,
>>> +						  u64 flags)
>>> +{
>>> +	static const char *name = "[kvm-gmem]";
>>> +	struct inode *inode;
>>> +	struct file *file;
>>> +	int err;
>>> +
>>> +	err = -ENOENT;
>>
>> Maybe add a comment here when the module reference will get
>> dropped. And maybe we should just switch to fops_get() + fops_put?
>>
>> /* __fput() will take care of fops_put(). */
>> if (!fops_get(&kvm_gmem_fops))
>> 	goto err;
>>
> 
> Sounds good! Please see attached patch. It's exactly what you suggested
> except I renamed the goto target to err_fops_put:
> 
>>> +
>>> +	inode = kvm_gmem_inode_make_secure_inode(name, size, flags);
>>> +	if (IS_ERR(inode)) {
>>> +		err = PTR_ERR(inode);
>>> +		goto err_put_module;
>>> +	}
>>> +
>>> +	file = alloc_file_pseudo(inode, kvm_gmem_mnt, name, O_RDWR,
>>> +				 &kvm_gmem_fops);
>>> +	if (IS_ERR(file)) {
>>> +		err = PTR_ERR(file);
>>> +		goto err_put_inode;
>>> +	}
>>> +
>>> +	file->f_flags |= O_LARGEFILE;
>>> +	file->private_data = priv;
>>> +
>>> +out:
>>> +	return file;
>>> +
>>> +err_put_inode:
>>> +	iput(inode);
>>> +err_put_module:
>>> +	module_put(kvm_gmem_fops.owner);
>>
>> fops_put(&kvm_gmem_fops);
>>
>> ?
>>
>>
>> Acked-by: David Hildenbrand <david@...hat.com>
>>
>> -- 
>> Cheers,
>>
>> David / dhildenb
> 
> From f2bd4499bce4db69bf34be75e009579db4329b7c Mon Sep 17 00:00:00 2001
> From: Ackerley Tng <ackerleytng@...gle.com>
> Date: Sun, 13 Jul 2025 17:43:35 +0000
> Subject: [PATCH] KVM: guest_memfd: Use guest mem inodes instead of anonymous
>  inodes
> 
> guest_memfd's inode represents memory the guest_memfd is
> providing. guest_memfd's file represents a struct kvm's view of that
> memory.
> 
> Using a custom inode allows customization of the inode teardown
> process via callbacks. For example, ->evict_inode() allows
> customization of the truncation process on file close, and
> ->destroy_inode() and ->free_inode() allow customization of the inode
> freeing process.
> 
> Customizing the truncation process allows flexibility in management of
> guest_memfd memory and customization of the inode freeing process
> allows proper cleanup of memory metadata stored on the inode.
> 
> Memory metadata is more appropriately stored on the inode (as opposed
> to the file), since the metadata is for the memory and is not unique
> to a specific binding and struct kvm.
> 
> Co-developed-by: Fuad Tabba <tabba@...gle.com>
> Signed-off-by: Fuad Tabba <tabba@...gle.com>
> Signed-off-by: Shivank Garg <shivankg@....com>
> Signed-off-by: Ackerley Tng <ackerleytng@...gle.com>
> ---
>  include/uapi/linux/magic.h |   1 +
>  virt/kvm/guest_memfd.c     | 129 ++++++++++++++++++++++++++++++-------
>  virt/kvm/kvm_main.c        |   7 +-
>  virt/kvm/kvm_mm.h          |   9 +--
>  4 files changed, 119 insertions(+), 27 deletions(-)
> 
> diff --git a/include/uapi/linux/magic.h b/include/uapi/linux/magic.h
> index bb575f3ab45e5..638ca21b7a909 100644
> --- a/include/uapi/linux/magic.h
> +++ b/include/uapi/linux/magic.h
> @@ -103,5 +103,6 @@
>  #define DEVMEM_MAGIC		0x454d444d	/* "DMEM" */
>  #define SECRETMEM_MAGIC		0x5345434d	/* "SECM" */
>  #define PID_FS_MAGIC		0x50494446	/* "PIDF" */
> +#define GUEST_MEMFD_MAGIC	0x474d454d	/* "GMEM" */
> 
>  #endif /* __LINUX_MAGIC_H__ */
> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index 08a6bc7d25b60..6c66a09740550 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c
> @@ -1,12 +1,16 @@
>  // SPDX-License-Identifier: GPL-2.0
> +#include <linux/anon_inodes.h>
>  #include <linux/backing-dev.h>
>  #include <linux/falloc.h>
> +#include <linux/fs.h>
>  #include <linux/kvm_host.h>
> +#include <linux/pseudo_fs.h>
>  #include <linux/pagemap.h>
> -#include <linux/anon_inodes.h>
> 
>  #include "kvm_mm.h"
> 
> +static struct vfsmount *kvm_gmem_mnt;
> +
>  struct kvm_gmem {
>  	struct kvm *kvm;
>  	struct xarray bindings;
> @@ -385,9 +389,45 @@ static struct file_operations kvm_gmem_fops = {
>  	.fallocate	= kvm_gmem_fallocate,
>  };
> 
> -void kvm_gmem_init(struct module *module)
> +static int kvm_gmem_init_fs_context(struct fs_context *fc)
> +{
> +	if (!init_pseudo(fc, GUEST_MEMFD_MAGIC))
> +		return -ENOMEM;
> +
> +	fc->s_iflags |= SB_I_NOEXEC;
> +	fc->s_iflags |= SB_I_NODEV;
> +
> +	return 0;
> +}
> +
> +static struct file_system_type kvm_gmem_fs = {
> +	.name		 = "guest_memfd",
> +	.init_fs_context = kvm_gmem_init_fs_context,
> +	.kill_sb	 = kill_anon_super,
> +};
> +
> +static int kvm_gmem_init_mount(void)
> +{
> +	kvm_gmem_mnt = kern_mount(&kvm_gmem_fs);
> +
> +	if (IS_ERR(kvm_gmem_mnt))
> +		return PTR_ERR(kvm_gmem_mnt);
> +
> +	kvm_gmem_mnt->mnt_flags |= MNT_NOEXEC;
> +	return 0;
> +}
> +
> +int kvm_gmem_init(struct module *module)
>  {
>  	kvm_gmem_fops.owner = module;
> +
> +	return kvm_gmem_init_mount();
> +}
> +
> +void kvm_gmem_exit(void)
> +{
> +	kern_unmount(kvm_gmem_mnt);
> +	kvm_gmem_mnt = NULL;
>  }
> 
>  static int kvm_gmem_migrate_folio(struct address_space *mapping,
> @@ -463,11 +503,72 @@ bool __weak kvm_arch_supports_gmem_mmap(struct kvm *kvm)
>  	return true;
>  }
> 
> +static struct inode *kvm_gmem_inode_make_secure_inode(const char *name,
> +						      loff_t size, u64 flags)
> +{
> +	struct inode *inode;
> +
> +	inode = anon_inode_make_secure_inode(kvm_gmem_mnt->mnt_sb, name, NULL);
> +	if (IS_ERR(inode))
> +		return inode;
> +
> +	inode->i_private = (void *)(unsigned long)flags;
> +	inode->i_op = &kvm_gmem_iops;
> +	inode->i_mapping->a_ops = &kvm_gmem_aops;
> +	inode->i_mode |= S_IFREG;
> +	inode->i_size = size;
> +	mapping_set_gfp_mask(inode->i_mapping, GFP_HIGHUSER);
> +	mapping_set_inaccessible(inode->i_mapping);
> +	/* Unmovable mappings are supposed to be marked unevictable as well. */
> +	WARN_ON_ONCE(!mapping_unevictable(inode->i_mapping));
> +
> +	return inode;
> +}
> +
> +static struct file *kvm_gmem_inode_create_getfile(void *priv, loff_t size,
> +						  u64 flags)
> +{
> +	static const char *name = "[kvm-gmem]";
> +	struct inode *inode;
> +	struct file *file;
> +	int err;
> +
> +	err = -ENOENT;
> +	/* __fput() will take care of fops_put(). */
> +	if (!fops_get(&kvm_gmem_fops))
> +		goto err;
> +
> +	inode = kvm_gmem_inode_make_secure_inode(name, size, flags);
> +	if (IS_ERR(inode)) {
> +		err = PTR_ERR(inode);
> +		goto err_fops_put;
> +	}
> +
> +	file = alloc_file_pseudo(inode, kvm_gmem_mnt, name, O_RDWR,
> +				 &kvm_gmem_fops);
> +	if (IS_ERR(file)) {
> +		err = PTR_ERR(file);
> +		goto err_put_inode;
> +	}
> +
> +	file->f_flags |= O_LARGEFILE;
> +	file->private_data = priv;
> +
> +out:
> +	return file;
> +
> +err_put_inode:
> +	iput(inode);
> +err_fops_put:
> +	fops_put(&kvm_gmem_fops);

Thanks Ackerley.
LGTM

Powered by blists - more mailing lists