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

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

[Cen Zhang]

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;
+	}
+
 	/*
 	 * 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;
-	}
-
-	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

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

[Joseph Qi]

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