Re: [PATCH v7 09/42] KVM: guest_memfd: Add base support for KVM_SET_MEMORY_ATTRIBUTES2

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing .compat_ioctl for guest_memfd file operations breaks 32-bit userspace.
- [Medium] Silent truncation of 64-bit memory attributes in maple tree storage.
--

commit 49c89936f50d0c8e067175446db94838144dd770
Author: Ackerley Tng <ackerleytng@google.com>

KVM: guest_memfd: Add base support for KVM_SET_MEMORY_ATTRIBUTES2

This commit introduces base support for KVM_SET_MEMORY_ATTRIBUTES2 in
guest_memfd to update memory attributes. It defines the new ioctl and
its corresponding struct, allowing both read and write operations to
provide error details to userspace.

> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index 4f7c4824c3a45..91e89b188f583 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c
> @@ -540,11 +540,125 @@ unsigned long kvm_gmem_get_memory_attributes(struct kvm *kvm, gfn_t gfn)
[ ... ]
> +static int __kvm_gmem_set_attributes(struct inode *inode, pgoff_t start,
> +				     size_t nr_pages, uint64_t attrs)
> +{
[ ... ]
> +	kvm_gmem_invalidate_begin(inode, start, end);
> +	mas_store_prealloc(&mas, xa_mk_value(attrs));

[Severity: Medium]
Could the use of xa_mk_value() here silently truncate the highest bit of the
64-bit attributes field?

The UAPI defines attributes as a 64-bit integer (__u64), but xa_mk_value()
takes an unsigned long and shifts it left by 1 to set the lowest bit for
internal pointer tagging. On a 64-bit system, this restricts the usable
value to 63 bits. If an attribute ever uses bit 63, it seems it would be
lost when shifted, contradicting the 64-bit UAPI contract.

> +	kvm_gmem_invalidate_end(inode, start, end);
> +out:
> +	filemap_invalidate_unlock(mapping);
> +	return r;
> +}
[ ... ]
>  static struct file_operations kvm_gmem_fops = {
>  	.mmap		= kvm_gmem_mmap,
>  	.open		= generic_file_open,
>  	.release	= kvm_gmem_release,
>  	.fallocate	= kvm_gmem_fallocate,
> +	.unlocked_ioctl	= kvm_gmem_ioctl,

[Severity: High]
Does this missing .compat_ioctl handler break 32-bit userspace?

Without a .compat_ioctl handler, 32-bit KVM userspace tasks running on a
64-bit kernel will unconditionally receive -ENOTTY when attempting to use
the new KVM_SET_MEMORY_ATTRIBUTES2 ioctl. 

Since the layout of kvm_memory_attributes2 is identical across 32-bit and
64-bit architectures (all members are __u64), should this assign 
.compat_ioctl = compat_ptr_ioctl to ensure API compatibility for compat
tasks?

>  };
>  
>  static int kvm_gmem_migrate_folio(struct address_space *mapping,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260522-gmem-inplace-conversion-v7-0-2f0fae496530@google.com?part=9