Re: [PATCH RFC v2 05/50] KVM: guest_memfd: Support using resource provider

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] kvm: guest_memfd: kvm_gmem_create() checks resource_fd when flag is not set
- [High] KVM: guest_memfd: movable allocation of unmovable tmpfs-provided folios
- [Medium] KVM: guest_memfd: fget_raw() mismatch with f_op in kvm_gmem_attach_resource()
--

commit 82719b2c47dfb94fadcdf6f025718ccc05e83332
Author: Ackerley Tng <ackerleytng@google.com>

KVM: guest_memfd: Support using resource provider

This commit adds the GUEST_MEMFD_FLAG_USE_RESOURCE flag and resource_fd
parameter to KVM_CREATE_GUEST_MEMFD so that guest_memfd inodes can attach
to an external resource provider via resource_file->f_op->gmem_provider_ops
and release the provider on inode destruction.

> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 8dff2fc1972e9..f4108bf7e3192 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1674,11 +1674,14 @@ struct kvm_memory_attributes2 {
>  #define KVM_CREATE_GUEST_MEMFD	_IOWR(KVMIO,  0xd4, struct kvm_create_guest_memfd)
>  #define GUEST_MEMFD_FLAG_MMAP		(1ULL << 0)
>  #define GUEST_MEMFD_FLAG_INIT_SHARED	(1ULL << 1)
> +#define GUEST_MEMFD_FLAG_USE_RESOURCE	(1ULL << 2)
>  
>  struct kvm_create_guest_memfd {
>  	__u64 size;
>  	__u64 flags;
> -	__u64 reserved[6];
> +	__s32 resource_fd;
> +	__u32 pad;
> +	__u64 reserved[5];
>  };
>  
>  #define KVM_PRE_FAULT_MEMORY	_IOWR(KVMIO, 0xd5, struct kvm_pre_fault_memory)
> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index ee823f2ef0368..84877beed6701 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c
> @@ -85,15 +85,26 @@ static const struct guest_memfd_provider_operations gmem_native_provider_ops = {
>  static int kvm_gmem_attach_resource(struct inode *inode, int resource_fd)
>  {
>  	const struct guest_memfd_provider_operations *ops = NULL;
> +	void *provider = ERR_PTR(-EOPNOTSUPP);
>  	struct file *resource_file = NULL;
> -	void *provider;
>  
> -	if (resource_fd == GMEM_RESOURCE_FD_NATIVE)
> +	if (resource_fd == GMEM_RESOURCE_FD_NATIVE) {
>  		ops = &gmem_native_provider_ops;
> -	else
> -		return -EOPNOTSUPP;
> +	} else {
> +		resource_file = fget_raw(resource_fd);
> +		if (!resource_file)
> +			return -EBADF;
> +
> +		if (resource_file->f_op)
> +			ops = resource_file->f_op->gmem_provider_ops;
> +	}

[Severity: Medium]
Why does kvm_gmem_attach_resource() use fget_raw() here when looking up
resource_file, and then read ops from resource_file->f_op?

In fs/file.c, the only difference between fget() and fget_raw() is that
fget_raw() accepts O_PATH (FMODE_PATH) file descriptors:

fs/file.c:
    struct file *fget(unsigned int fd)
    {
        return __fget(fd, FMODE_PATH);
    }

    struct file *fget_raw(unsigned int fd)
    {
        return __fget(fd, 0);
    }

However, in fs/open.c:do_dentry_open(), any file opened with O_PATH (such
as a mount fd from fsmount() or open_tree(), or a directory opened with
O_PATH) has f->f_op set to &empty_fops rather than inode->i_fop:

fs/open.c:do_dentry_open() {
    ...
    if (unlikely(f->f_flags & O_PATH)) {
        f->f_mode = FMODE_PATH | FMODE_OPENED;
        file_set_fsnotify_mode(f, FMODE_NONOTIFY);
        f->f_op = &empty_fops;
        return 0;
    }
    ...
}

As a result, when userspace passes an O_PATH fd as resource_fd to
ioctl(KVM_CREATE_GUEST_MEMFD) with GUEST_MEMFD_FLAG_USE_RESOURCE,
resource_file->f_op->gmem_provider_ops is always NULL and attaching fails
with -EOPNOTSUPP.

If O_PATH descriptors are meant to be supported, should this read
gmem_provider_ops from file_inode(resource_file)->i_fop instead of
resource_file->f_op? Conversely, if O_PATH descriptors are not intended to
be supported, should this use fget(resource_fd) so O_PATH descriptors fail
with -EBADF (and drop the NULL check on resource_file->f_op)?

> +
> +	if (ops && ops->attach && ops->alloc_folio)
> +		provider = ops->attach(inode, resource_file);

[Severity: High]
When attaching a tmpfs resource provider via kvm_gmem_attach_resource(),
can allocating folios through ops->alloc_folio() place unmovable
guest_memfd pages into ZONE_MOVABLE or MIGRATE_CMA/MIGRATE_MOVABLE
pageblocks?

In virt/kvm/guest_memfd.c:kvm_gmem_init_inode(), guest_memfd inodes
configure GFP_HIGHUSER (without __GFP_MOVABLE) and mark the mapping as
inaccessible because guest_memfd folios are neither migratable nor
swappable:

virt/kvm/guest_memfd.c:kvm_gmem_init_inode() {
    ...
    mapping_set_gfp_mask(inode->i_mapping, GFP_HIGHUSER);

    /*
     * guest_memfd memory is neither migratable nor swappable: set
     * inaccessible to gate off both.
     */
    mapping_set_inaccessible(inode->i_mapping);
    ...
}

Native allocation in gmem_native_alloc_folio() uses
mapping_gfp_mask(gi->vfs_inode.i_mapping) (GFP_HIGHUSER). However, when a
tmpfs resource provider is attached and populated via
kvm_gmem_get_folio() -> gmem_provider_alloc_folio() ->
shmem_gmem_alloc_folio(), mm/shmem.c allocates the folio with
GFP_HIGHUSER_MOVABLE before kvm_gmem_get_folio() inserts it into the
AS_INACCESSIBLE guest_memfd mapping:

mm/shmem.c:shmem_gmem_alloc_folio() {
    ...
    if (mpol)
        folio = folio_alloc_mpol(GFP_HIGHUSER_MOVABLE, 0, mpol, index,
                                 numa_node_id());
    else
        folio = folio_alloc(GFP_HIGHUSER_MOVABLE, 0);
    ...
}

Because kvm_gmem_migrate_folio() rejects migration with -EINVAL and
isolate_migratepages_block() / move_to_new_folio() refuse to migrate
mapping_inaccessible() folios, allocating these folios with __GFP_MOVABLE
pins movable and CMA pageblocks and can cause memory hot-unplug
(offline_pages()) to fail or loop when attempting to migrate them.

Should gmem_provider_alloc_folio() pass mapping_gfp_mask(inode->i_mapping)
(or should shmem_gmem_alloc_folio() use GFP_HIGHUSER instead of
GFP_HIGHUSER_MOVABLE)?

[ ... ]

> @@ -1111,6 +1128,7 @@ static int __kvm_gmem_create(struct kvm *kvm, loff_t size, u64 flags)
>  
>  int kvm_gmem_create(struct kvm *kvm, struct kvm_create_guest_memfd *args)
>  {
> +	int resource_fd = GMEM_RESOURCE_FD_NATIVE;
>  	loff_t size = args->size;
>  	u64 flags = args->flags;
>  
> @@ -1120,7 +1138,15 @@ int kvm_gmem_create(struct kvm *kvm, struct kvm_create_guest_memfd *args)
>  	if (size <= 0 || !PAGE_ALIGNED(size))
>  		return -EINVAL;
>  
> -	return __kvm_gmem_create(kvm, size, flags);
> +	if (flags & GUEST_MEMFD_FLAG_USE_RESOURCE) {
> +		if (args->resource_fd < 0)
> +			return -EBADF;
> +		resource_fd = args->resource_fd;
> +	} else if (args->resource_fd) {
> +		return -EINVAL;
> +	}

[Severity: High]
Could checking args->resource_fd when GUEST_MEMFD_FLAG_USE_RESOURCE is not
set cause a userspace ABI regression for existing KVM_CREATE_GUEST_MEMFD
callers?

Prior to this change, struct kvm_create_guest_memfd in
include/uapi/linux/kvm.h had __u64 reserved[6] after flags, and neither
kvm_vm_ioctl() nor kvm_gmem_create() verified that reserved[] was zeroed
(and Documentation/virt/kvm/api.rst did not require reserved[] to be zero).

Because resource_fd is carved out of reserved[0] and gated by
GUEST_MEMFD_FLAG_USE_RESOURCE, any existing userspace caller that allocates
struct kvm_create_guest_memfd on the stack and only initializes .size and
.flags (or initializes unused fd fields to -1) without setting
GUEST_MEMFD_FLAG_USE_RESOURCE will now fail with -EINVAL in
kvm_gmem_create(). Meanwhile, args->pad and args->reserved[0..4] remain
unchecked.

Should args->resource_fd only be inspected when
GUEST_MEMFD_FLAG_USE_RESOURCE is set in flags (similar to how
KVM_SET_USER_MEMORY_REGION2 only inspects guest_memfd and
guest_memfd_offset when KVM_MEM_GUEST_MEMFD is set)?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-gmem-alternative-providers-v2-0-5dd1dc6c660a@google.com?part=5