Re: [PATCH v7 3/9] KVM: guest_memfd: Allow host to map guest_memfd() pages

[Shivank Garg <shivankg@amd.com>]

[-- Attachment #1: Type: text/plain, Size: 4125 bytes --]



On 4/11/2025 4:14 AM, Ackerley Tng wrote:
> Shivank Garg <shivankg@amd.com> writes:
> 
>> On 4/8/2025 10:28 PM, Ackerley Tng wrote:
>>> Shivank Garg <shivankg@amd.com> 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@google.com>
>>>>
>>>> ...
>>>> ...
>>>>> +
>>>>> +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.

[-- Attachment #2: 0001-KVM-guest_memfd-Remove-redundant-kvm_gmem_getattr.patch --]
[-- Type: text/plain, Size: 1272 bytes --]

From bf713f4b7d96dc92960d24537b488582c5521722 Mon Sep 17 00:00:00 2001
From: Shivank Garg <shivankg@amd.com>
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@amd.com>
---
 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