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

[Ackerley Tng <ackerleytng@google.com>]

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.

>
> 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 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