Re: [PATCH 01/16] xfs: fix chown leaking delalloc quota blocks when fssetxattr fails

[Brian Foster <bfoster@redhat.com>]

On Tue, Feb 02, 2021 at 09:47:26AM -0800, Darrick J. Wong wrote:
> On Tue, Feb 02, 2021 at 08:13:15AM -0500, Brian Foster wrote:
> > On Mon, Feb 01, 2021 at 06:03:23PM -0800, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > While refactoring the quota code to create a function to allocate inode
> > > change transactions, I noticed that xfs_qm_vop_chown_reserve does more
> > > than just make reservations: it also *modifies* the incore counts
> > > directly to handle the owner id change for the delalloc blocks.
> > > 
> > > I then observed that the fssetxattr code continues validating input
> > > arguments after making the quota reservation but before dirtying the
> > > transaction.  If the routine decides to error out, it fails to undo the
> > > accounting switch!  This leads to incorrect quota reservation and
> > > failure down the line.
> > > 
> > > We can fix this by making the reservation function do only that -- for
> > > the new dquot, it reserves ondisk and delalloc blocks to the
> > > transaction, and the old dquot hangs on to its incore reservation for
> > > now.  Once we actually switch the dquots, we can then update the incore
> > > reservations because we've dirtied the transaction and it's too late to
> > > turn back now.
> > > 
> > > No fixes tag because this has been broken since the start of git.
> > > 
> > 
> > Do we have any test coverage for this problem?
> 
> Working on it, yeah.  The tricky part is finding a combination of
> FSSETXATTR values that will reliably trigger an error return.  I think
> it can be done by allocating delalloc blocks and then trying to set an
> extent size hint and change the project id in a single call.
> 
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > > ---
> > >  fs/xfs/xfs_qm.c |   91 +++++++++++++++++++++----------------------------------
> > >  1 file changed, 34 insertions(+), 57 deletions(-)
> > > 
> > > 
> > > diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> > > index c134eb4aeaa8..322d337b5dca 100644
> > > --- a/fs/xfs/xfs_qm.c
> > > +++ b/fs/xfs/xfs_qm.c
> > > @@ -1785,6 +1785,28 @@ xfs_qm_vop_chown(
> > >  	xfs_trans_mod_dquot(tp, newdq, bfield, ip->i_d.di_nblocks);
> > >  	xfs_trans_mod_dquot(tp, newdq, XFS_TRANS_DQ_ICOUNT, 1);
> > >  
> > > +	/*
> > > +	 * Back when we made quota reservations for the chown, we reserved the
> > > +	 * ondisk blocks + delalloc blocks with the new dquot.  Now that we've
> > > +	 * switched the dquots, decrease the new dquot's block reservation
> > > +	 * (having already bumped up the real counter) so that we don't have
> > > +	 * any reservation to give back when we commit.
> > > +	 */
> > > +	xfs_trans_mod_dquot(tp, newdq, XFS_TRANS_DQ_RES_BLKS,
> > > +			-ip->i_delayed_blks);
> > > +
> > 
> > Ok, so the reservation code code below continues to reserve di_blocks
> > and delalloc blocks as it did before, but associates it with the
> > transaction and no longer reduces delalloc reservation from the old
> > dquots. Instead, this function increases the consumption of reserved
> > blocks for di_blocks and then removes the additional quota reservation
> > from the transaction, but not the new dquots. Thus when the transaction
> > commits, this has the side effect of leaving additional reservation on
> > the new dquots that correspond to the delalloc blocks on the inode. Am I
> > following that correctly?
> 
> Correct.
> 

Thanks.

> > > +	/*
> > > +	 * Give the incore reservation for delalloc blocks back to the old
> > > +	 * dquot.  We don't normally handle delalloc quota reservations
> > > +	 * transactionally, so just lock the dquot and subtract from the
> > > +	 * reservation.  We've dirtied the transaction, so it's too late to
> > > +	 * turn back now.
> > > +	 */
> > > +	tp->t_flags |= XFS_TRANS_DIRTY;
> > > +	xfs_dqlock(prevdq);
> > > +	prevdq->q_blk.reserved -= ip->i_delayed_blks;
> > > +	xfs_dqunlock(prevdq);
> > > +
> > 
> > What's the reason for not using xfs_trans_reserve_quota_bydquots(NULL,
> > ...) here like the original code?
> 
> xfs_trans_reserve_quota_bydquots() makes the caller pass in user, group,
> and project dquots.  It's not difficult to add more code to declare and
> route parameters, but that just felt overdone.
> 

Ah, Ok.

> Given that this is the only place in the codebase where we want to
> change the incore quota reservation on a single dquot, I also didn't
> think it was worth making a whole new function.
> 
> FWIW I don't really mind doing it, it just seemed like more work.
> Alternately I suppose I could expose xfs_trans_dqresv.
> 

I have no strong preference. It seems fine to me as it is, the reason
for the change just wasn't clear to me at first glance:

Reviewed-by: Brian Foster <bfoster@redhat.com>

> --D
> 
> > Brian
> > 
> > >  	/*
> > >  	 * Take an extra reference, because the inode is going to keep
> > >  	 * this dquot pointer even after the trans_commit.
> > > @@ -1807,84 +1829,39 @@ xfs_qm_vop_chown_reserve(
> > >  	uint			flags)
> > >  {
> > >  	struct xfs_mount	*mp = ip->i_mount;
> > > -	uint64_t		delblks;
> > >  	unsigned int		blkflags;
> > > -	struct xfs_dquot	*udq_unres = NULL;
> > > -	struct xfs_dquot	*gdq_unres = NULL;
> > > -	struct xfs_dquot	*pdq_unres = NULL;
> > >  	struct xfs_dquot	*udq_delblks = NULL;
> > >  	struct xfs_dquot	*gdq_delblks = NULL;
> > >  	struct xfs_dquot	*pdq_delblks = NULL;
> > > -	int			error;
> > > -
> > >  
> > >  	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED));
> > >  	ASSERT(XFS_IS_QUOTA_RUNNING(mp));
> > >  
> > > -	delblks = ip->i_delayed_blks;
> > >  	blkflags = XFS_IS_REALTIME_INODE(ip) ?
> > >  			XFS_QMOPT_RES_RTBLKS : XFS_QMOPT_RES_REGBLKS;
> > >  
> > >  	if (XFS_IS_UQUOTA_ON(mp) && udqp &&
> > > -	    i_uid_read(VFS_I(ip)) != udqp->q_id) {
> > > +	    i_uid_read(VFS_I(ip)) != udqp->q_id)
> > >  		udq_delblks = udqp;
> > > -		/*
> > > -		 * If there are delayed allocation blocks, then we have to
> > > -		 * unreserve those from the old dquot, and add them to the
> > > -		 * new dquot.
> > > -		 */
> > > -		if (delblks) {
> > > -			ASSERT(ip->i_udquot);
> > > -			udq_unres = ip->i_udquot;
> > > -		}
> > > -	}
> > > +
> > >  	if (XFS_IS_GQUOTA_ON(ip->i_mount) && gdqp &&
> > > -	    i_gid_read(VFS_I(ip)) != gdqp->q_id) {
> > > +	    i_gid_read(VFS_I(ip)) != gdqp->q_id)
> > >  		gdq_delblks = gdqp;
> > > -		if (delblks) {
> > > -			ASSERT(ip->i_gdquot);
> > > -			gdq_unres = ip->i_gdquot;
> > > -		}
> > > -	}
> > >  
> > >  	if (XFS_IS_PQUOTA_ON(ip->i_mount) && pdqp &&
> > > -	    ip->i_d.di_projid != pdqp->q_id) {
> > > +	    ip->i_d.di_projid != pdqp->q_id)
> > >  		pdq_delblks = pdqp;
> > > -		if (delblks) {
> > > -			ASSERT(ip->i_pdquot);
> > > -			pdq_unres = ip->i_pdquot;
> > > -		}
> > > -	}
> > > -
> > > -	error = xfs_trans_reserve_quota_bydquots(tp, ip->i_mount,
> > > -				udq_delblks, gdq_delblks, pdq_delblks,
> > > -				ip->i_d.di_nblocks, 1, flags | blkflags);
> > > -	if (error)
> > > -		return error;
> > >  
> > >  	/*
> > > -	 * Do the delayed blks reservations/unreservations now. Since, these
> > > -	 * are done without the help of a transaction, if a reservation fails
> > > -	 * its previous reservations won't be automatically undone by trans
> > > -	 * code. So, we have to do it manually here.
> > > +	 * Reserve enough quota to handle blocks on disk and reserved for a
> > > +	 * delayed allocation.  We'll actually transfer the delalloc
> > > +	 * reservation between dquots at chown time, even though that part is
> > > +	 * only semi-transactional.
> > >  	 */
> > > -	if (delblks) {
> > > -		/*
> > > -		 * Do the reservations first. Unreservation can't fail.
> > > -		 */
> > > -		ASSERT(udq_delblks || gdq_delblks || pdq_delblks);
> > > -		ASSERT(udq_unres || gdq_unres || pdq_unres);
> > > -		error = xfs_trans_reserve_quota_bydquots(NULL, ip->i_mount,
> > > -			    udq_delblks, gdq_delblks, pdq_delblks,
> > > -			    (xfs_qcnt_t)delblks, 0, flags | blkflags);
> > > -		if (error)
> > > -			return error;
> > > -		xfs_trans_reserve_quota_bydquots(NULL, ip->i_mount,
> > > -				udq_unres, gdq_unres, pdq_unres,
> > > -				-((xfs_qcnt_t)delblks), 0, blkflags);
> > > -	}
> > > -
> > > -	return 0;
> > > +	return xfs_trans_reserve_quota_bydquots(tp, ip->i_mount, udq_delblks,
> > > +			gdq_delblks, pdq_delblks,
> > > +			ip->i_d.di_nblocks + ip->i_delayed_blks,
> > > +			1, blkflags | flags);
> > >  }
> > >  
> > >  int
> > > 
> > 
>