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