On 4/11/2025 4:14 AM, Ackerley Tng wrote:
> Shivank Garg <shivankg@xxxxxxx> writes:
>
>> On 4/8/2025 10:28 PM, Ackerley Tng wrote:
>>> Shivank Garg <shivankg@xxxxxxx> writes:
>>>
>>>> Hi Fuad,
>>>>
>>>> On 3/18/2025 9:48 PM, Fuad Tabba wrote:
>>>>> Add support for mmap() and fault() for guest_memfd backed memory
>>>>> in the host for VMs that support in-place conversion between
>>>>> shared and private. To that end, this patch adds the ability to
>>>>> check whether the VM type supports in-place conversion, and only
>>>>> allows mapping its memory if that's the case.
>>>>>
>>>>> Also add the KVM capability KVM_CAP_GMEM_SHARED_MEM, which
>>>>> indicates that the VM supports shared memory in guest_memfd, or
>>>>> that the host can create VMs that support shared memory.
>>>>> Supporting shared memory implies that memory can be mapped when
>>>>> shared with the host.
>>>>>
>>>>> This is controlled by the KVM_GMEM_SHARED_MEM configuration
>>>>> option.
>>>>>
>>>>> Signed-off-by: Fuad Tabba <tabba@xxxxxxxxxx>
>>>>
>>>> ...
>>>> ...
>>>>> +
>>>>> +static int kvm_gmem_mmap(struct file *file, struct vm_area_struct *vma)
>>>>> +{
>>>>> + struct kvm_gmem *gmem = file->private_data;
>>>>> +
>>>>> + if (!kvm_arch_gmem_supports_shared_mem(gmem->kvm))
>>>>> + return -ENODEV;
>>>>> +
>>>>> + if ((vma->vm_flags & (VM_SHARED | VM_MAYSHARE)) !=
>>>>> + (VM_SHARED | VM_MAYSHARE)) {
>>>>> + return -EINVAL;
>>>>> + }
>>>>> +
>>>>> + file_accessed(file);
>>>>
>>>> As it is not directly visible to userspace, do we need to update the
>>>> file's access time via file_accessed()?
>>>>
>>>
>>> Could you explain a little more about this being directly visible to
>>> userspace?
>>>
>>> IIUC generic_fillattr(), which guest_memfd uses, will fill stat->atime
>>> from the inode's atime. file_accessed() will update atime and so this
>>> should be userspace accessible. (Unless I missed something along the way
>>> that blocks the update)
>>>
>>
>> By visibility to userspace, I meant that guest_memfd is in-memory and not
>> directly exposed to users as a traditional file would be.
>
> shmem is also in-memory and uses updates atime, so I guess being
> in-memory doesn't mean we shouldn't update atime.
>
> guest_memfd is not quite traditional, but would the mmap patches Fuad is
> working on now qualify the guest_memfd as more traditional?
>
>>
>> Yes, theoretically atime is accessible to user, but is it actually useful for
>> guest_memfd, and do users track atime in this context? In my understanding,
>> this might be an unnecessary unless we want to maintain it for VFS consistency.
>>
>> My analysis of the call flow:
>> fstat() -> vfs_fstat() -> vfs_getattr() -> vfs_getattr_nosec() -> kvm_gmem_getattr()
>> I couldn't find any kernel-side consumers of inode's atime or instances where
>> it's being used for any internal purposes.
>>
>> Searching for examples, I found secretmem_mmap() skips file_accessed().
>>
>
> I guess I'm okay both ways, I don't think I have a use case for reading
> atime, but I assumed VFS consistency is a good thing.
I'm happy to go with whatever maintainers think best for file_accessed().
>>
>> Also as side note, I believe the kvm_gmem_getattr() ops implementation
>> might be unnecessary here.
>> Since kvm_gmem_getattr() is simply calling generic_fillattr() without
>> any special handling, couldn't we just use the default implementation?
>>
>> int vfs_getattr_nosec(const struct path *path, struct kstat *stat,
>> u32 request_mask, unsigned int query_flags)
>> {
>> ...
>> if (inode->i_op->getattr)
>> return inode->i_op->getattr(idmap, path, stat,
>> request_mask,
>> query_flags);
>>
>> generic_fillattr(idmap, request_mask, inode, stat);
>> return 0;
>> }
>
> I noticed this too. I agree that we could actually just use
> generic_fillattr() by not specifying ->getattr().
I’ve attached a patch to remove it, letting the VFS default
handle attribute queries. Please let me know if it looks good or
needs tweaks.
From bf713f4b7d96dc92960d24537b488582c5521722 Mon Sep 17 00:00:00 2001
From: Shivank Garg <shivankg@xxxxxxx>
Date: Fri, 11 Apr 2025 08:06:21 +0000
Subject: [PATCH] KVM: guest_memfd: Remove redundant kvm_gmem_getattr
Drop kvm_gmem_getattr, which only calls generic_fillattr, and
rely on the VFS default implementation to simplify the code.
Signed-off-by: Shivank Garg <shivankg@xxxxxxx>
---
virt/kvm/guest_memfd.c | 11 -----------
1 file changed, 11 deletions(-)
diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
index b2aa6bf24d3a..7d85cc33c0bb 100644
--- a/virt/kvm/guest_memfd.c
+++ b/virt/kvm/guest_memfd.c
@@ -382,23 +382,12 @@ static const struct address_space_operations kvm_gmem_aops = {
#endif
};
-static int kvm_gmem_getattr(struct mnt_idmap *idmap, const struct path *path,
- struct kstat *stat, u32 request_mask,
- unsigned int query_flags)
-{
- struct inode *inode = path->dentry->d_inode;
-
- generic_fillattr(idmap, request_mask, inode, stat);
- return 0;
-}
-
static int kvm_gmem_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
struct iattr *attr)
{
return -EINVAL;
}
static const struct inode_operations kvm_gmem_iops = {
- .getattr = kvm_gmem_getattr,
.setattr = kvm_gmem_setattr,
};
--
2.34.1
