Re: [RFC PATCH v8 5/7] KVM: guest_memfd: Add slab-allocated inode cache

[Shivank Garg <shivankg@xxxxxxx>]

On 6/24/2025 9:46 AM, Huang, Ying wrote:
> Shivank Garg <shivankg@xxxxxxx> writes:
> 
>> +static void kvm_gmem_init_inodecache(void)
>> +{
>> +	kvm_gmem_inode_cachep = kmem_cache_create("kvm_gmem_inode_cache",
>> +						  sizeof(struct kvm_gmem_inode_info),
>> +						  0, SLAB_ACCOUNT,
>> +						  kvm_gmem_init_inode);
> 
> Check the return value?
> 
> And, I'm not a big fan of (logically) one line function encapsulation.
> 
>> +}
>> +
>>  int kvm_gmem_init(struct module *module)
>>  {
>>  	kvm_gmem_fops.owner = module;
>>  
>> +	kvm_gmem_init_inodecache();
>>  	return kvm_gmem_init_mount();
> 
> kmem_cache_destroy(kvm_gmem_inode_cachep) if kvm_gmem_init_mount()
> return with error?
> 

Thanks for the feedback, Ying.
Good catch on the leak!

Regarding the missing error check, I noticed while looking at examples that 
kernel code is sometimes inconsistent with kmem_cache_create() error handling, 
but you're right about checking for failures, So I'll handle them properly.

diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
index 7b8c548986d4..8cb83692f7a0 100644
--- a/virt/kvm/guest_memfd.c
+++ b/virt/kvm/guest_memfd.c
@@ -530,20 +530,23 @@ static void kvm_gmem_init_inode(void *foo)
        inode_init_once(&info->vfs_inode);
 }

-static void kvm_gmem_init_inodecache(void)
+int kvm_gmem_init(struct module *module)
 {
+       int ret;
+
+       kvm_gmem_fops.owner = module;
        kvm_gmem_inode_cachep = kmem_cache_create("kvm_gmem_inode_cache",
                                                  sizeof(struct kvm_gmem_inode_info),
                                                  0, SLAB_ACCOUNT,
                                                  kvm_gmem_init_inode);
-}
-
-int kvm_gmem_init(struct module *module)
-{
-       kvm_gmem_fops.owner = module;
-
-       kvm_gmem_init_inodecache();
-       return kvm_gmem_init_mount();
+       if (!kvm_gmem_inode_cachep)
+               return -ENOMEM;
+       ret = kvm_gmem_init_mount();
+       if (ret) {
+               kmem_cache_destroy(kvm_gmem_inode_cachep);
+               return ret;
+       }
+       return 0;
 }

Best Regards,
Shivank