Re: [PATCH v4 10/23] KVM: x86: Map a separate VMA for each address space for internal memslots

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] KVM: x86: deferred init of __free(fput) in __x86_set_memory_region()
--

commit cfd5ddf3a5baa5fbd0b9d7dc48b7ac7a5d553771
Author: Sean Christopherson <seanjc@google.com>

KVM: x86: Map a separate VMA for each address space for internal memslots

This commit maps a separate VMA for each address space when installing
internal memslots by creating an explicit shmem file via
shmem_kernel_file_setup() so both memslots' VMAs point at the same backing
page, allowing each address space's memslot VMA to be unmapped independently.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 0b94f981a2e1c..8cb58040dd81a 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c

[ ... ]

> @@ -10047,7 +10062,8 @@ static int x86_set_internal_memslot(struct kvm *kvm, int id, int as_id,
>  void __user * __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa,
>  				      u32 size)
>  {
> -	unsigned long hva;
> +	struct file *f __free(fput) = NULL;
> +	struct kvm_memory_slot *slot;
>  	int i, r;
>  
>  	lockdep_assert_held(&kvm->slots_lock);
> @@ -10055,37 +10071,31 @@ void __user * __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa,
>  	if (WARN_ON(id >= KVM_MEM_SLOTS_NUM))
>  		return ERR_PTR_USR(-EINVAL);
>  
> -	if (size && id_to_memslot(kvm_memslots(kvm), id))
> -		return ERR_PTR_USR(-EEXIST);
> -
>  	if (size) {
> -		/*
> -		 * MAP_SHARED to prevent internal slot pages from being moved
> -		 * by fork()/COW.
> -		 */
> -		hva = vm_mmap(NULL, 0, size, PROT_READ | PROT_WRITE,
> -			      MAP_SHARED | MAP_ANONYMOUS, 0);
> -		if (IS_ERR_VALUE(hva))
> -			return (void __user *)hva;
> -	} else {
> -		hva = 0;
> +		if (id_to_memslot(kvm_memslots(kvm), id))
> +			return ERR_PTR_USR(-EEXIST);
> +
> +		f = shmem_kernel_file_setup("kvm_memslot", size, EMPTY_VMA_FLAGS);
> +		if (IS_ERR(f))
> +			return (void __user *)f;
>  	}

[Severity: Low]
This isn't a bug, but in __x86_set_memory_region() the __free(fput) variable f
is declared and initialized to NULL at the top of the function and then
assigned later inside the if (size) block, whereas include/linux/cleanup.h
documents that __free() variables should be defined and initialized in a
single statement to preserve reverse-declaration cleanup ordering.

If the id_to_memslot(kvm_memslots(kvm), id) check is kept before the
declaration of f, could f be defined and initialized in a single statement,
for example:

	struct file *f __free(fput) = size ?
		shmem_kernel_file_setup("kvm_memslot", size, EMPTY_VMA_FLAGS) :
		NULL;
	if (IS_ERR(f))
		return (void __user *)f;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030212112.2598640-1-seanjc@google.com?part=10