Yan Zhao <yan.y.z...@intel.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-ta...@google.com/
