| 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
| ||
|
Message-ID: <diqz4iui4y00.fsf@ackerleytng-ctop.c.googlers.com>
Date: Thu, 07 Aug 2025 14:34:23 -0700
From: Ackerley Tng <ackerleytng@...gle.com>
To: David Hildenbrand <david@...hat.com>, Shivank Garg <shivankg@....com>, seanjc@...gle.com,
vbabka@...e.cz, willy@...radead.org, akpm@...ux-foundation.org,
shuah@...nel.org, pbonzini@...hat.com, brauner@...nel.org,
viro@...iv.linux.org.uk
Cc: paul@...l-moore.com, jmorris@...ei.org, serge@...lyn.com, pvorel@...e.cz,
bfoster@...hat.com, tabba@...gle.com, vannapurve@...gle.com,
chao.gao@...el.com, bharata@....com, nikunj@....com, michael.day@....com,
shdhiman@....com, yan.y.zhao@...el.com, Neeraj.Upadhyay@....com,
thomas.lendacky@....com, michael.roth@....com, aik@....com, jgg@...dia.com,
kalyazin@...zon.com, peterx@...hat.com, jack@...e.cz, rppt@...nel.org,
hch@...radead.org, cgzones@...glemail.com, ira.weiny@...el.com,
rientjes@...gle.com, roypat@...zon.co.uk, ziy@...dia.com,
matthew.brost@...el.com, joshua.hahnjy@...il.com, rakie.kim@...com,
byungchul@...com, gourry@...rry.net, kent.overstreet@...ux.dev,
ying.huang@...ux.alibaba.com, apopple@...dia.com, chao.p.peng@...el.com,
amit@...radead.org, ddutile@...hat.com, dan.j.williams@...el.com,
ashish.kalra@....com, gshan@...hat.com, jgowans@...zon.com,
pankaj.gupta@....com, papaluri@....com, yuzhao@...gle.com,
suzuki.poulose@....com, quic_eberman@...cinc.com,
aneeshkumar.kizhakeveetil@....com, linux-fsdevel@...r.kernel.org,
linux-mm@...ck.org, linux-kernel@...r.kernel.org,
linux-security-module@...r.kernel.org, kvm@...r.kernel.org,
linux-kselftest@...r.kernel.org, linux-coco@...ts.linux.dev
Subject: Re: [PATCH V9 1/7] KVM: guest_memfd: Use guest mem inodes instead of
anonymous inodes
David Hildenbrand <david@...hat.com> writes:
> On 13.07.25 19:43, Shivank Garg wrote:
>> From: Ackerley Tng <ackerleytng@...gle.com>
>>
>> guest_memfd's inode represents memory the guest_memfd is
>> providing. guest_memfd's file represents a struct kvm's view of that
>> memory.
>>
>> Using a custom inode allows customization of the inode teardown
>> process via callbacks. For example, ->evict_inode() allows
>> customization of the truncation process on file close, and
>> ->destroy_inode() and ->free_inode() allow customization of the inode
>> freeing process.
>>
>> Customizing the truncation process allows flexibility in management of
>> guest_memfd memory and customization of the inode freeing process
>> allows proper cleanup of memory metadata stored on the inode.
>>
>> Memory metadata is more appropriately stored on the inode (as opposed
>> to the file), since the metadata is for the memory and is not unique
>> to a specific binding and struct kvm.
>>
>> Co-developed-by: Fuad Tabba <tabba@...gle.com>
>> Signed-off-by: Fuad Tabba <tabba@...gle.com>
>> Signed-off-by: Ackerley Tng <ackerleytng@...gle.com>
>> Signed-off-by: Shivank Garg <shivankg@....com>
>> ---
>
> [...]
>
>>
>> #include "kvm_mm.h"
>>
>> +static struct vfsmount *kvm_gmem_mnt;
>> +
>> struct kvm_gmem {
>> struct kvm *kvm;
>> struct xarray bindings;
>> @@ -388,9 +392,51 @@ static struct file_operations kvm_gmem_fops = {
>> .fallocate = kvm_gmem_fallocate,
>> };
>>
>> -void kvm_gmem_init(struct module *module)
>> +static const struct super_operations kvm_gmem_super_operations = {
>> + .statfs = simple_statfs,
>> +};
>> +
>> +static int kvm_gmem_init_fs_context(struct fs_context *fc)
>> +{
>> + struct pseudo_fs_context *ctx;
>> +
>> + if (!init_pseudo(fc, GUEST_MEMFD_MAGIC))
>> + return -ENOMEM;
>> +
>> + ctx = fc->fs_private;
>> + ctx->ops = &kvm_gmem_super_operations;
>
> Curious, why is that required? (secretmem doesn't have it, so I wonder)
>
Good point! pseudo_fs_fill_super() fills in a struct super_operations
which already does simple_statfs, so guest_memfd doesn't need this.
>> +
>> + return 0;
>> +}
>> +
>> +static struct file_system_type kvm_gmem_fs = {
>> + .name = "kvm_guest_memory",
>
> It's GUEST_MEMFD_MAGIC but here "kvm_guest_memory".
>
> For secretmem it's SECRETMEM_MAGIC vs. "secretmem".
>
> So naturally, I wonder if that is to be made consistent :)
>
I'll update this to "guest_memfd" to be consistent.
>> + .init_fs_context = kvm_gmem_init_fs_context,
>> + .kill_sb = kill_anon_super,
>> +};
>> +
>> +static int kvm_gmem_init_mount(void)
>> +{
>> + kvm_gmem_mnt = kern_mount(&kvm_gmem_fs);
>> +
>> + if (IS_ERR(kvm_gmem_mnt))
>> + return PTR_ERR(kvm_gmem_mnt);
>> +
>> + kvm_gmem_mnt->mnt_flags |= MNT_NOEXEC;
>> + return 0;
>> +}
>> +
>> +int kvm_gmem_init(struct module *module)
>> {
>> kvm_gmem_fops.owner = module;
>> +
>> + return kvm_gmem_init_mount();
>> +}
>> +
>> +void kvm_gmem_exit(void)
>> +{
>> + kern_unmount(kvm_gmem_mnt);
>> + kvm_gmem_mnt = NULL;
>> }
>>
>> static int kvm_gmem_migrate_folio(struct address_space *mapping,
>> @@ -472,11 +518,71 @@ static const struct inode_operations kvm_gmem_iops = {
>> .setattr = kvm_gmem_setattr,
>> };
>>
>> +static struct inode *kvm_gmem_inode_make_secure_inode(const char *name,
>> + loff_t size, u64 flags)
>> +{
>> + struct inode *inode;
>> +
>> + inode = anon_inode_make_secure_inode(kvm_gmem_mnt->mnt_sb, name, NULL);
>> + if (IS_ERR(inode))
>> + return inode;
>> +
>> + inode->i_private = (void *)(unsigned long)flags;
>> + inode->i_op = &kvm_gmem_iops;
>> + inode->i_mapping->a_ops = &kvm_gmem_aops;
>> + inode->i_mode |= S_IFREG;
>> + inode->i_size = size;
>> + mapping_set_gfp_mask(inode->i_mapping, GFP_HIGHUSER);
>> + mapping_set_inaccessible(inode->i_mapping);
>> + /* Unmovable mappings are supposed to be marked unevictable as well. */
>> + WARN_ON_ONCE(!mapping_unevictable(inode->i_mapping));
>> +
>> + return inode;
>> +}
>> +
>> +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;
>> + int err;
>> +
>> + err = -ENOENT;
>> + if (!try_module_get(kvm_gmem_fops.owner))
>> + goto err;
>
> Curious, shouldn't there be a module_put() somewhere after this function
> returned a file?
>
This was interesting indeed, but IIUC this is correct.
I think this flow was basically copied from __anon_inode_getfile(),
which does this try_module_get().
The corresponding module_put() is in __fput(), which calls fops_put()
and calls module_put() on the owner.
>> +
>> + inode = kvm_gmem_inode_make_secure_inode(name, size, flags);
>> + if (IS_ERR(inode)) {
>> + err = PTR_ERR(inode);
>> + goto err_put_module;
>> + }
>> +
>> + file = alloc_file_pseudo(inode, kvm_gmem_mnt, name, O_RDWR,
>> + &kvm_gmem_fops);
>> + if (IS_ERR(file)) {
>> + err = PTR_ERR(file);
>> + goto err_put_inode;
>> + }
>> +
>> + file->f_flags |= O_LARGEFILE;
>> + file->private_data = priv;
>> +
>>
>
> Nothing else jumped at me.
>
Thanks for the review!
Since we're going to submit this patch through Shivank's mempolicy
support series, I'll follow up soon by sending a replacement patch in
reply to this series so Shivank could build on top of that?
> --
> Cheers,
>
> David / dhildenb
Powered by blists - more mailing lists