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

[Shivank Garg <shivankg@amd.com>]

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.

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().


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 might be missing some context. Please feel free to correct me
if I've misunderstood anything.

Thanks,
Shivank

>>> +	vm_flags_set(vma, VM_DONTDUMP);
>>> +	vma->vm_ops = &kvm_gmem_vm_ops;
>>> +
>>> +	return 0;
>>> +}
>>> +#else
>>> +#define kvm_gmem_mmap NULL
>>> +#endif /* CONFIG_KVM_GMEM_SHARED_MEM */
>>> +
>>>  static struct file_operations kvm_gmem_fops = {
>>> +	.mmap		= kvm_gmem_mmap,
>>>  	.open		= generic_file_open,
>>>  	.release	= kvm_gmem_release,
>>>  	.fallocate	= kvm_gmem_fallocate,
>>
>> Thanks,
>> Shivank