Re: [PATCH RFC v2 06/50] KVM: selftests: Add helper to create guest_memfd with a resource file

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] KVM: guest_memfd: missing release_folio() with mapping_set_release_always()
- [Medium] mm: shmem: unmovable guest_memfd folios allocated with GFP_HIGHUSER_MOVABLE
--

commit 27616bb0c537e1022b72cd23715bdb3c99505f19
Author: Ackerley Tng <ackerleytng@google.com>

KVM: selftests: Add helper to create guest_memfd with a resource file

This commit updates the tools UAPI header for KVM_CREATE_GUEST_MEMFD to
define GUEST_MEMFD_FLAG_USE_RESOURCE and resource_fd, and adds selftest
helpers to create a guest_memfd backed by a resource file descriptor.

> diff --git a/tools/include/uapi/linux/kvm.h b/tools/include/uapi/linux/kvm.h
> index 419011097fa8e..66c1f40b50c29 100644
> --- a/tools/include/uapi/linux/kvm.h
> +++ b/tools/include/uapi/linux/kvm.h
> @@ -1654,11 +1654,14 @@ struct kvm_memory_attributes {
>  #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)

[Severity: High]
To support provider folio invalidation for guest_memfd in this series,
virt/kvm/guest_memfd.c:kvm_gmem_init_inode() enables AS_RELEASE_ALWAYS so
that truncate_cleanup_folio() sees folio_needs_release(folio) as true and
invokes kvm_gmem_invalidate_folio():

virt/kvm/guest_memfd.c:kvm_gmem_init_inode() {
	...
	mapping_set_inaccessible(inode->i_mapping);
	WARN_ON_ONCE(!mapping_unevictable(inode->i_mapping));
	mapping_set_release_always(inode->i_mapping);
}

However, kvm_gmem_aops in virt/kvm/guest_memfd.c does not define a
.release_folio callback:

static const struct address_space_operations kvm_gmem_aops = {
	.dirty_folio = noop_dirty_folio,
	.migrate_folio	= kvm_gmem_migrate_folio,
	.error_remove_folio = kvm_gmem_error_folio,
	.invalidate_folio = kvm_gmem_invalidate_folio,
#ifdef CONFIG_HAVE_KVM_ARCH_GMEM_RECLAIM
	.free_folio = kvm_gmem_free_folio,
#endif
};

When folio_needs_release(folio) is true because of AS_RELEASE_ALWAYS and
mapping->a_ops->release_folio is NULL, mm/filemap.c:filemap_release_folio()
falls back to fs/buffer.c:try_to_free_buffers(), which hits a warning on
guest_memfd folios and returns true:

mm/filemap.c:filemap_release_folio() {
	if (mapping && mapping->a_ops->release_folio)
		return mapping->a_ops->release_folio(folio, gfp);
	return try_to_free_buffers(folio);
}

fs/buffer.c:try_to_free_buffers() {
	/* Misconfigured folio check */
	if (WARN_ON_ONCE(!folio_buffers(folio)))
		return true;
	...
}

If soft_offline_page() -> mm/memory-failure.c:soft_offline_in_use_page()
(via MADV_SOFT_OFFLINE, sysfs soft_offline_page, or corrected memory error
handling) runs on a clean, unmapped guest_memfd folio (which has PG_lru set
on LRU_UNEVICTABLE by filemap_add_folio() -> folio_add_lru()), or if
mm/memory.c:__do_fault() handles a PageHWPoison fault, it calls
mapping_evict_folio():

mm/memory-failure.c:soft_offline_in_use_page() {
	...
	if (!huge && folio_test_lru(folio) && !folio_test_swapcache(folio))
		/*
		 * Try to invalidate first. This should work for
		 * non dirty unmapped page cache pages.
		 */
		ret = mapping_evict_folio(folio_mapping(folio), folio);
}

Can this cause mapping_evict_folio() -> filemap_release_folio() to trigger
WARN_ON_ONCE(!folio_buffers(folio)) and then call remove_mapping(), evicting
and freeing the folio without calling kvm_gmem_invalidate_folio() or
kvm_gmem_invalidate_start() / kvm_gmem_invalidate_end(), leaking the tmpfs
provider sbinfo->used_blocks accounting and leaving stale stage-2 mappings
pointing to freed pages?

Should kvm_gmem_aops define a .release_folio callback to prevent
filemap_release_folio() from falling back to try_to_free_buffers() and
evicting folios without invalidation?

> diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
> index e2618877619e1..eca296539db79 100644
> --- a/tools/testing/selftests/kvm/include/kvm_util.h
> +++ b/tools/testing/selftests/kvm/include/kvm_util.h
> @@ -809,26 +809,39 @@ static inline bool is_smt_on(void)
>  
>  void vm_create_irqchip(struct kvm_vm *vm);
>  
> -static inline int __vm_create_guest_memfd(struct kvm_vm *vm, u64 size,
> -					  u64 flags)
> +static inline int __vm_create_guest_memfd_resource(struct kvm_vm *vm, u64 size,
> +						   u64 flags, int resource_fd)
>  {
>  	struct kvm_create_guest_memfd guest_memfd = {
>  		.size = size,
>  		.flags = flags,
> +		.resource_fd = resource_fd,
>  	};
>  
>  	return __vm_ioctl(vm, KVM_CREATE_GUEST_MEMFD, &guest_memfd);
>  }

[Severity: Medium]
When creating a guest_memfd with GUEST_MEMFD_FLAG_USE_RESOURCE and a tmpfs
resource_fd via __vm_create_guest_memfd_resource(), faulting or fallocating
pages calls kvm_gmem_get_folio() -> gmem_provider_alloc_folio() ->
mm/shmem.c:shmem_gmem_alloc_folio():

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);
	...
}

In virt/kvm/guest_memfd.c:kvm_gmem_init_inode(), the guest_memfd mapping is
configured with GFP_HIGHUSER (without __GFP_MOVABLE) and marked
AS_INACCESSIBLE because guest_memfd folios cannot be migrated:

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);
	...
}

Unlike gmem_native_alloc_folio(), which uses
mapping_gfp_mask(gi->vfs_inode.i_mapping), shmem_gmem_alloc_folio() passes
GFP_HIGHUSER_MOVABLE. Once kvm_gmem_get_folio() inserts the folio into
inode->i_mapping via filemap_add_folio(), isolate_migratepages_block(),
move_to_new_folio(), and kvm_gmem_migrate_folio() all reject migration on
the AS_INACCESSIBLE mapping.

Could allocating these unmovable folios with GFP_HIGHUSER_MOVABLE place them
in ZONE_MOVABLE or MIGRATE_CMA pageblocks and permanently pin those blocks
against contiguous allocations and memory hot-unplug?

[ ... ]

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