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
