Re: [PATCH v2] ocfs2: validate move extents against a stable EOF

[Joseph Qi <joseph.qi@linux.alibaba.com>]

On 10/25/26 12:20 AM, Cen Zhang wrote:
> The move-extents range must be bounded by the inode size used for extent
> movement. ocfs2_ioctl_move_extents() checks and clamps the request before
> taking the local inode and cluster locks in ocfs2_move_extents(). It also
> uses separate i_size_read() calls for the range check and subtraction.
> 
> A concurrent truncate can invalidate the first check and make the later
> unsigned subtraction underflow:
> 
>   Move-extents ioctl                  Truncate
>   ------------------                  --------
>   check me_start <= i_size
>                                       take inode and cluster locks
>                                       shrink EOF below me_start
>                                       release locks
>   compare me_start + me_len with EOF
>   set me_len = EOF - me_start
>     [unsigned underflow]
>   acquire inode and cluster locks
>   select extents using the bad range
> 
> A truncate after clamping can also leave the range extending beyond EOF.
> Conversely, the cached size may be smaller than the size refreshed when
> the cluster inode lock is acquired. Early validation can then reject a
> valid start or permanently shorten the requested length and threshold.
> The me_start + me_len comparison can also wrap for a large input length.
> 
> Validate and adjust the original request once, after inode_lock(), the
> exclusive rw lock and ocfs2_inode_lock() have stabilized and refreshed
> the size. Read EOF once, reject a start beyond it, and bound the length
> by EOF minus start. Normalize the threshold using the same EOF snapshot.
> This keeps zero as the default-threshold sentinel until validation uses
> the size that will govern extent movement.
> 
> Move goal validation after range adjustment as well. Its allocation-group
> capacity check uses me_len and must see the EOF-clamped length. Both
> validation failures use the existing inode-lock unwind path before any
> extent movement or transaction starts.
> 
> Fixes: 53069d4e7695 ("Ocfs2/move_extents: move/defrag extents within a certain range.")
> Cc: stable@vger.kernel.org
> Assisted-by: LLM
> Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
> ---
> Changes in v2:
> - Validate and normalize the original range only once, after inode and
>   cluster locking have stabilized and refreshed EOF.
> - Move goal validation after range adjustment because its allocation-group
>   capacity check depends on me_len.
> 
> Link to v1: https://lore.kernel.org/r/pm-ocfs2-move-extents-isize-rrw-v3-b2a7508c41108cc2d00b@gmail.com
> 
>  fs/ocfs2/move_extents.c | 68 ++++++++++++++++++++++-------------------
>  1 file changed, 37 insertions(+), 31 deletions(-)
> 
> diff --git a/fs/ocfs2/move_extents.c b/fs/ocfs2/move_extents.c
> index ad1678ee7cc4..f2682ef6bf2b 100644
> --- a/fs/ocfs2/move_extents.c
> +++ b/fs/ocfs2/move_extents.c
> @@ -907,6 +907,30 @@ out:
>  	return ret;
>  }
>  
> +static int ocfs2_validate_and_adjust_move_range(struct inode *inode,
> +						struct ocfs2_move_extents *range)
> +{
> +	u64 i_size = i_size_read(inode);
> +
> +	if (range->me_start > i_size)
> +		return -EINVAL;
> +
> +	if (range->me_len > i_size - range->me_start)
> +		range->me_len = i_size - range->me_start;
> +
> +	/*
> +	 * The default threshold for defragmentation is 1 MiB, which
> +	 * matches the maximum OCFS2 cluster size.
> +	 */
> +	if (!range->me_threshold)
> +		range->me_threshold = 1024 * 1024;
> +
> +	if (range->me_threshold > i_size)
> +		range->me_threshold = i_size;
> +
> +	return 0;
> +}
> +
>  static int ocfs2_move_extents(struct ocfs2_move_extents_context *context)
>  {
>  	int status;
> @@ -936,6 +960,19 @@ static int ocfs2_move_extents(struct ocfs2_move_extents_context *context)
>  		goto out_rw_unlock;
>  	}
>  
> +	status = ocfs2_validate_and_adjust_move_range(inode, context->range);
> +	if (status) {
> +		mlog_errno(status);
> +		goto out_inode_unlock;
> +	}
> +
> +	/* The goal check depends on the adjusted range length. */
> +	if (!context->auto_defrag) {
> +		status = ocfs2_validate_and_adjust_move_goal(inode, context->range);
> +		if (status)
> +			goto out_inode_unlock;

Seems we'd better log the failure like ocfs2_validate_and_adjust_move_range().

> +	}
> +
>  	/*
>  	 * remember ip_xattr_sem also needs to be held if necessary
>  	 */
> @@ -1028,27 +1065,8 @@ int ocfs2_ioctl_move_extents(struct file *filp, void __user *argp)
>  		goto out_free;
>  	}
>  
> -	if (range.me_start > i_size_read(inode)) {
> -		status = -EINVAL;
> -		goto out_free;
> -	}

This seems changes the return value. It returned -EINVAL without touching
the caller's buffer. It now unwinds through out_inode_unlock and falls into
the copy back and returns -EFAULT instead of -EINVAL.

Seems not a bug since the goal validation failure already behaved this way,
but worth a line in the commit log.

Thanks,
Joseph

> -
> -	if (range.me_start + range.me_len > i_size_read(inode))
> -			range.me_len = i_size_read(inode) - range.me_start;
> -
>  	context->range = &range;
>  
> -	/*
> -	 * ok, the default threshold for the defragmentation
> -	 * is 1M, since our maximum clustersize was 1M also.
> -	 * any thought?
> -	 */
> -	if (!range.me_threshold)
> -		range.me_threshold = 1024 * 1024;
> -
> -	if (range.me_threshold > i_size_read(inode))
> -		range.me_threshold = i_size_read(inode);
> -
>  	if (range.me_flags & ~(OCFS2_MOVE_EXT_FL_AUTO_DEFRAG |
>  			       OCFS2_MOVE_EXT_FL_PART_DEFRAG)) {
>  		status = -EINVAL;
> @@ -1060,23 +1078,11 @@ int ocfs2_ioctl_move_extents(struct file *filp, void __user *argp)
>  
>  		if (range.me_flags & OCFS2_MOVE_EXT_FL_PART_DEFRAG)
>  			context->partial = 1;
> -	} else {
> -		/*
> -		 * first best-effort attempt to validate and adjust the goal
> -		 * (physical address in block), while it can't guarantee later
> -		 * operation can succeed all the time since global_bitmap may
> -		 * change a bit over time.
> -		 */
> -
> -		status = ocfs2_validate_and_adjust_move_goal(inode, &range);
> -		if (status)
> -			goto out_copy;
>  	}
>  
>  	status = ocfs2_move_extents(context);
>  	if (status)
>  		mlog_errno(status);
> -out_copy:
>  	/*
>  	 * movement/defragmentation may end up being partially completed,
>  	 * that's the reason why we need to return userspace the finished