Re: [PATCH 02/12] xfs: check deferred refcount op continuation parameters

[Dave Chinner <david@fromorbit.com>]

On Thu, Oct 27, 2022 at 10:14:14AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> If we're in the middle of a deferred refcount operation and decide to
> roll the transaction to avoid overflowing the transaction space, we need
> to check the new agbno/aglen parameters that we're about to record in
> the new intent.  Specifically, we need to check that the new extent is
> completely within the filesystem, and that continuation does not put us
> into a different AG.
> 
> If the keys of a node block are wrong, the lookup to resume an
> xfs_refcount_adjust_extents operation can put us into the wrong record
> block.  If this happens, we might not find that we run out of aglen at
> an exact record boundary, which will cause the loop control to do the
> wrong thing.
> 
> The previous patch should take care of that problem, but let's add this
> extra sanity check to stop corruption problems sooner than later.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/libxfs/xfs_refcount.c |   48 ++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 46 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_refcount.c b/fs/xfs/libxfs/xfs_refcount.c
> index 831353ba96dc..c6aa832a8713 100644
> --- a/fs/xfs/libxfs/xfs_refcount.c
> +++ b/fs/xfs/libxfs/xfs_refcount.c
> @@ -1138,6 +1138,44 @@ xfs_refcount_finish_one_cleanup(
>  		xfs_trans_brelse(tp, agbp);
>  }
>  
> +/*
> + * Set up a continuation a deferred refcount operation by updating the intent.
> + * Checks to make sure we're not going to run off the end of the AG.
> + */
> +static inline int
> +xfs_refcount_continue_op(
> +	struct xfs_btree_cur		*cur,
> +	xfs_fsblock_t			startblock,
> +	xfs_agblock_t			new_agbno,
> +	xfs_extlen_t			new_len,
> +	xfs_fsblock_t			*fsbp)
> +{
> +	struct xfs_mount		*mp = cur->bc_mp;
> +	struct xfs_perag		*pag = cur->bc_ag.pag;
> +	xfs_fsblock_t			new_fsbno;
> +	xfs_agnumber_t			old_agno;
> +
> +	old_agno = XFS_FSB_TO_AGNO(mp, startblock);
> +	new_fsbno = XFS_AGB_TO_FSB(mp, pag->pag_agno, new_agbno);
> +
> +	/*
> +	 * If we don't have any work left to do, then there's no need
> +	 * to perform the validation of the new parameters.
> +	 */
> +	if (!new_len)
> +		goto done;

Shouldn't we be validating new_fsbno rather than just returning
whatever we calculated here?

> +	if (XFS_IS_CORRUPT(mp, !xfs_verify_fsbext(mp, new_fsbno, new_len)))
> +		return -EFSCORRUPTED;
> +
> +	if (XFS_IS_CORRUPT(mp, old_agno != XFS_FSB_TO_AGNO(mp, new_fsbno)))
> +		return -EFSCORRUPTED;

We already know what agno new_fsbno sits in - we calculated it
directly from pag->pag_agno above, so this can jsut check against
pag->pag_agno directly, right?

i.e.

	if (XFS_IS_CORRUPT(mp,
			XFS_FSB_TO_AGNO(mp, startblock) != pag->pag_agno))
		return -EFSCORRUPTED;

and we don't need the local variable for it....

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com