Re: [RFC PATCH 13/39] KVM: guest_memfd: Make guest mem use guest mem inodes instead of anonymous inodes

[Yan Zhao]

Hi Ackerley,

Not sure if below nits have been resolved in your latest code.
I came across them and felt it's better to report them anyway.

Apologies for any redundancy if you've already addressed them.

On Tue, Sep 10, 2024 at 11:43:44PM +0000, Ackerley Tng wrote:
> +static void kvm_gmem_init_mount(void)                                         
> +{                                                                             
> +     kvm_gmem_mnt = kern_mount(&kvm_gmem_fs);                                 
> +     BUG_ON(IS_ERR(kvm_gmem_mnt));                                            
> +                                                                              
> +     /* For giggles. Userspace can never map this anyways. */                 
> +     kvm_gmem_mnt->mnt_flags |= MNT_NOEXEC;                                   
> +}                                                                             
> +                                                                              
>  static struct file_operations kvm_gmem_fops = {                               
>       .open           = generic_file_open,                                     
>       .release        = kvm_gmem_release,                                      
> @@ -311,6 +348,8 @@ static struct file_operations kvm_gmem_fops = {            
>  void kvm_gmem_init(struct module *module)                                     
>  {                                                                             
>       kvm_gmem_fops.owner = module;                                            
> +                                                                              
> +     kvm_gmem_init_mount();                                                   
>  } 
When KVM is compiled as a module, looks "kern_unmount(kvm_gmem_mnt)" is
missing in the kvm_exit() path.

This may lead to kernel oops when executing "sync" after KVM is unloaded or
reloaded.

BTW, there're lots of symbols not exported under mm.

> +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;
> +
> +	if (kvm_gmem_fops.owner && !try_module_get(kvm_gmem_fops.owner))
> +		return ERR_PTR(-ENOENT);
> +
> +	inode = kvm_gmem_inode_make_secure_inode(name, size, flags);
> +	if (IS_ERR(inode))
Missing module_put() here. i.e.,

-       if (IS_ERR(inode))
+       if (IS_ERR(inode)) {
+               if (kvm_gmem_fops.owner)
+                       module_put(kvm_gmem_fops.owner);
+
                return ERR_CAST(inode);
+       }

> +		return ERR_CAST(inode);
> +
> +	file = alloc_file_pseudo(inode, kvm_gmem_mnt, name, O_RDWR,
> +				 &kvm_gmem_fops);
> +	if (IS_ERR(file)) {
> +		iput(inode);
> +		return file;
> +	}
> +
> +	file->f_mapping = inode->i_mapping;
> +	file->f_flags |= O_LARGEFILE;
> +	file->private_data = priv;
> +
> +	return file;
> +}
> +

Thanks
Yan