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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z+y2nU7KDmRpuISM@yzhao56-desk.sh.intel.com>
Date: Wed, 2 Apr 2025 12:01:33 +0800
From: Yan Zhao <yan.y.zhao@...el.com>
To: Ackerley Tng <ackerleytng@...gle.com>
CC: <tabba@...gle.com>, <quic_eberman@...cinc.com>, <roypat@...zon.co.uk>,
	<jgg@...dia.com>, <peterx@...hat.com>, <david@...hat.com>,
	<rientjes@...gle.com>, <fvdl@...gle.com>, <jthoughton@...gle.com>,
	<seanjc@...gle.com>, <pbonzini@...hat.com>, <zhiquan1.li@...el.com>,
	<fan.du@...el.com>, <jun.miao@...el.com>, <isaku.yamahata@...el.com>,
	<muchun.song@...ux.dev>, <mike.kravetz@...cle.com>, <erdemaktas@...gle.com>,
	<vannapurve@...gle.com>, <qperret@...gle.com>, <jhubbard@...dia.com>,
	<willy@...radead.org>, <shuah@...nel.org>, <brauner@...nel.org>,
	<bfoster@...hat.com>, <kent.overstreet@...ux.dev>, <pvorel@...e.cz>,
	<rppt@...nel.org>, <richard.weiyang@...il.com>, <anup@...infault.org>,
	<haibo1.xu@...el.com>, <ajones@...tanamicro.com>, <vkuznets@...hat.com>,
	<maciej.wieczor-retman@...el.com>, <pgonda@...gle.com>,
	<oliver.upton@...ux.dev>, <linux-kernel@...r.kernel.org>,
	<linux-mm@...ck.org>, <kvm@...r.kernel.org>,
	<linux-kselftest@...r.kernel.org>, <linux-fsdevel@...ck.org>
Subject: Re: [RFC PATCH 13/39] KVM: guest_memfd: Make guest mem use guest mem
 inodes instead of anonymous inodes

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