Re: [PATCH v5 35/36] KVM: guest_memfd: bind backing memory to a NUMA node at creation

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Unconditional validation of flag-gated syscall parameters outside their flag scope breaks ABI compatibility for existing userspace.
- [Medium] Missing lock when calling `mpol_set_nodemask` leads to data race and torn read of `current->mems_allowed`.
--

--- Patch [35]: [PATCH v5 35/36] KVM: guest_memfd: bind backing memory to a NUMA node at creation ---
commit 45130876f6e87292f19cee2648796441ea4622ce
Author: Gregory Price <[email protected]>

KVM: guest_memfd: bind backing memory to a NUMA node at creation

guest_memfd presently allocates its page-cache folios through a per-inode
shared mempolicy. This patch adds GUEST_MEMFD_FLAG_BIND_NODE to set an
MPOL_BIND policy for the requested node and install it over the whole
inode's shared policy.

> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index f0e5da4908660..de320df08e0ff 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c

[ ... ]

> @@ -456,6 +456,26 @@ static struct mempolicy *kvm_gmem_get_policy(struct vm_area_struct *vma,
>  	 */
>  	return mpol_shared_policy_lookup(&GMEM_I(inode)->policy, pgoff);
>  }
> +
> +static int kvm_gmem_bind_node(struct inode *inode, int node)
> +{
> +	struct mempolicy *pol;
> +	int err;
> +
> +	pol = mpol_bind_node(node);

[Severity: Medium]
Can calling mpol_bind_node() here without holding task_lock(current)
cause a data race?

Looking at mpol_bind_node(), it calls __mpol_bind_node() which then calls
mpol_set_nodemask(). This accesses current->mems_allowed locklessly.

If a task's cpuset is concurrently modified, this could lead to a torn read
of current->mems_allowed on systems with >64 NUMA nodes, potentially
failing or creating a policy with an incorrect or empty nodemask.

> +	if (IS_ERR(pol))
> +		return PTR_ERR(pol);
> +
> +	err = mpol_set_shared_policy_range(&GMEM_I(inode)->policy, 0,
> +					  MAX_LFS_FILESIZE >> PAGE_SHIFT, pol);
> +	mpol_put(pol);
> +	return err;
> +}

[ ... ]

> @@ -630,6 +656,7 @@ int kvm_gmem_create(struct kvm *kvm, struct kvm_create_guest_memfd *args)
>  {
>  	loff_t size = args->size;
>  	u64 flags = args->flags;
> +	int node = NUMA_NO_NODE;
>  
>  	if (flags & ~kvm_gmem_get_supported_flags(kvm))
>  		return -EINVAL;
> @@ -637,7 +664,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_BIND_NODE) {
> +		if (args->pad || args->node >= MAX_NUMNODES)
> +			return -EINVAL;
> +		node = args->node;
> +	} else if (args->node || args->pad) {

[Severity: Critical]
Does unconditionally checking args->node and args->pad when the
GUEST_MEMFD_FLAG_BIND_NODE flag is absent break ABI compatibility?

Since these fields were previously part of the unused reserved[] array,
existing userspace binaries might allocate struct kvm_create_guest_memfd
on the stack without zeroing them. 

If an older binary calls the ioctl without the new flag, and the stack
garbage happens to be non-zero, this will now fail with -EINVAL.

> +		return -EINVAL;
> +	}
> +
> +	return __kvm_gmem_create(kvm, size, flags, node);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=35