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: <diqzselyzl07.fsf@ackerleytng-ctop.c.googlers.com>
Date: Wed, 23 Apr 2025 13:22:00 -0700
From: Ackerley Tng <ackerleytng@...gle.com>
To: Yan Zhao <yan.y.zhao@...el.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

Yan Zhao <yan.y.zhao@...el.com> writes:

> 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.

No worries, thank you so much for your reviews!

>
> 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.
>

Thanks, Fuad will be addressing this in a revision of [1].

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

Thanks again, is there a good way to do a build test for symbols not
being exported?  What CONFIG flags do you use?

>> +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);
> +       }
>

Thanks, Fuad will be addressing this in a revision of [1].

>> +		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

[1] https://lore.kernel.org/all/20250328153133.3504118-2-tabba@google.com/