Re: [PATCH v3 5/9] kvm: guest_memfd: Add support for freezing and unfreezing mappings

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Global SRCU read lock held across unbounded fallocate operations causes system-wide DoS for guest_memfd freezing.

Pre-existing issues:
- [High] Calling `file_modified(file)` without holding the `inode_lock()`.
--

commit 76fd5a6866a1e58bea715f4038d1ddf2c323cc9d
Author: Tarun Sahu <tarunsahu@google.com>

kvm: guest_memfd: Add support for freezing and unfreezing mappings

This commit adds a freeze state on guest_memfd inodes to prevent fallocate and 
new page faults during VM live updates. It synchronizes these freezing states 
using a system-wide SRCU lock.

> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index fe1adc9ba48d1..a4d9d348e4d16 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c
[ ... ]
> @@ -273,16 +284,30 @@ static long kvm_gmem_allocate(struct inode *inode, loff_t offset, loff_t len)
>  static long kvm_gmem_fallocate(struct file *file, int mode, loff_t offset,
>  			       loff_t len)
>  {
> +	struct inode *inode = file_inode(file);
>  	int ret;
> +	int idx;
>  
> -	if (!(mode & FALLOC_FL_KEEP_SIZE))
> -		return -EOPNOTSUPP;
> +	idx = srcu_read_lock(&kvm_gmem_freeze_srcu);
> +	if (kvm_gmem_is_frozen(inode)) {
> +		srcu_read_unlock(&kvm_gmem_freeze_srcu, idx);
> +		return -EPERM;
> +	}
>  
> -	if (mode & ~(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE))
> -		return -EOPNOTSUPP;
> +	if (!(mode & FALLOC_FL_KEEP_SIZE)) {
> +		ret = -EOPNOTSUPP;
> +		goto out;
> +	}
>  
> -	if (!PAGE_ALIGNED(offset) || !PAGE_ALIGNED(len))
> -		return -EINVAL;
> +	if (mode & ~(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE)) {
> +		ret = -EOPNOTSUPP;
> +		goto out;
> +	}
> +
> +	if (!PAGE_ALIGNED(offset) || !PAGE_ALIGNED(len)) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
>  
>  	if (mode & FALLOC_FL_PUNCH_HOLE)
>  		ret = kvm_gmem_punch_hole(file_inode(file), offset, len);
>  	else
>  		ret = kvm_gmem_allocate(file_inode(file), offset, len);

[Severity: High]
Could holding this global SRCU read lock across potentially unbounded
operations cause a system-wide denial of service?

When kvm_gmem_fallocate() calls kvm_gmem_allocate() or
kvm_gmem_punch_hole(), the operation iterates over a potentially massive
memory region. While kvm_gmem_allocate() yields the CPU via cond_resched(),
it does not drop this global SRCU read lock.

If another task on the host concurrently calls kvm_gmem_freeze() to preserve
a different VM, its synchronize_srcu() could block indefinitely waiting for
this tenant's operation to complete. Should the SRCU lock be dropped
and reacquired during long loops to prevent cross-tenant starvation?

>  
>  	if (!ret)
>  		file_modified(file);

[Severity: High]
This is a pre-existing issue, but does calling file_modified() here violate
the VFS locking requirements?

The VFS API requires the caller of file_modified() to hold the file's
inode_lock (or i_rwsem) because it performs non-atomic timestamp updates
and security privilege clearing. Neither vfs_fallocate() nor
kvm_gmem_fallocate() appears to acquire the inode lock before this call.

> +
> +out:
> +	srcu_read_unlock(&kvm_gmem_freeze_srcu, idx);
>  	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260622184851.2309827-1-tarunsahu@google.com?part=5