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

["Darrick J. Wong" <djwong@kernel.org>]

On Fri, Oct 28, 2022 at 07:49:57AM +1100, Dave Chinner wrote:
> 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?

No.  Imagine that the deferred work is performed against the last 30
blocks of the last AG in the filesystem.  Let's say that the last AG is
AG 3 and the AG has 100 blocks.  fsblock 3:99 is the last fsblock in the
filesystem.

Before we start the deferred work, startblock == 3:70 and
blockcount == 30.  We adjust the refcount of those 30 blocks, so we're
done now.  The adjust function passes out new_agbno == 70 + 30 and
new_len == 30 - 30.

The agbno to fsbno conversion sets new_fsbno to 3:100 and new_len is 0.
However, fsblock 3/100 is one block past the end of both AG 3 and the
filesystem, so the check below will fail:

> > +	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?

We don't actually know what agno new_fsbno sits in because of the way
that the agblock -> fsblock conversion works:

#define XFS_AGB_TO_FSB(mp,agno,agbno)	\
	(((xfs_fsblock_t)(agno) << (mp)->m_sb.sb_agblklog) | (agbno))

Notice how we don't mask off the bits of agbno above sb_agblklog?  If
sb_agblklog is (say) 20 but agbno has bit 31 set, that bit 31 will bump
the AG number by 2^11 AGs.

The genesis of this patch was from the old days when rc_startblock had
bit 31 set on COW staging extents.  Splitting out the cow/shared domain
later in the series makes this patch sort of redundant, but I still want
to catch the cases where agbno has some bit set above agblklog due to
corruption/bitflips/shenanigans.

> 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....

Not quite -- we need to compare new_fsbno's agnumber against the perag
structure.  But you're right that @old_agno isn't necessary.

	if (XFS_IS_CORRUPT(mp,
			XFS_FSB_TO_AGNO(mp, new_fsbno) != pag->pag_agno))
		/* fail */

I'll change it to the above.

--D

> Cheers,
> 
> Dave.
> 
> -- 
> Dave Chinner
> david@fromorbit.com