Re: [PATCH v2] ocfs2: validate move extents against a stable EOF
From: Joseph Qi
Date: Mon Oct 26 2026 - 06:10:32 EST
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@xxxxxxxxxxxxxxx
> Assisted-by: LLM
> Signed-off-by: Cen Zhang <zzzccc427@xxxxxxxxx>
> ---
> 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@xxxxxxxxx
>
> 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 = ⦥
>
> - /*
> - * 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