[PATCH 0/3] xfs: random SWAPEXT fixes

[PATCH 0/3] xfs: random SWAPEXT fixes

[Darrick J. Wong]

Hi all,

This series fixes some problems I found in the extent swap code.  The
first patch makes sure we drop the inode lock when cleaning out the COW
fork fails.  The second patch turns some metadata ASSERTs into a proper
EFSCORRUPTED return.  The third patch teaches swapext to adjust the
quota counts properly, which fixes a bug where unprivileged userspace
can cause the quota counts to be wrong by calling swapext on a pair of
files with different uid/gid/prid.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=random-fixes

xfsprogs git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=random-fixes

fstests git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfstests-dev.git/log/?h=random-fixes

[PATCH 1/3] xfs: clean up the error handling in xfs_swap_extents

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Make sure we release resources properly if we cannot clean out the COW
extents in preparation for an extent swap.

Fixes: 96987eea537d6c ("xfs: cancel COW blocks before swapext")
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_bmap_util.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index 4f800f7fe888..cc23a3e23e2d 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -1606,7 +1606,7 @@ xfs_swap_extents(
 	if (xfs_inode_has_cow_data(tip)) {
 		error = xfs_reflink_cancel_cow_range(tip, 0, NULLFILEOFF, true);
 		if (error)
-			return error;
+			goto out_unlock;
 	}
 
 	/*

Re: [PATCH 1/3] xfs: clean up the error handling in xfs_swap_extents

[Christoph Hellwig]

On Mon, May 04, 2020 at 06:10:16PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Make sure we release resources properly if we cannot clean out the COW
> extents in preparation for an extent swap.

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 2/3] xfs: clean up the metadata validation in xfs_swap_extent_rmap

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Bail out if there's something not right with either file's fork
mappings.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_bmap_util.c |   31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)


diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index cc23a3e23e2d..2774939e176d 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -1342,8 +1342,16 @@ xfs_swap_extent_rmap(
 				&nimaps, 0);
 		if (error)
 			goto out;
-		ASSERT(nimaps == 1);
-		ASSERT(tirec.br_startblock != DELAYSTARTBLOCK);
+		if (nimaps != 1 || tirec.br_startblock == DELAYSTARTBLOCK) {
+			/*
+			 * We should never get no mapping or a delalloc extent
+			 * since the donor file should have been flushed by the
+			 * caller.
+			 */
+			ASSERT(0);
+			error = -EINVAL;
+			goto out;
+		}
 
 		trace_xfs_swap_extent_rmap_remap(tip, &tirec);
 		ilen = tirec.br_blockcount;
@@ -1360,8 +1368,17 @@ xfs_swap_extent_rmap(
 					&nimaps, 0);
 			if (error)
 				goto out;
-			ASSERT(nimaps == 1);
-			ASSERT(tirec.br_startoff == irec.br_startoff);
+			if (nimaps != 1 ||
+			    tirec.br_startoff != irec.br_startoff) {
+				/*
+				 * We should never get no mapping or a mapping
+				 * for another offset, but bail out if that
+				 * ever does.
+				 */
+				ASSERT(0);
+				error = -EFSCORRUPTED;
+				goto out;
+			}
 			trace_xfs_swap_extent_rmap_remap_piece(ip, &irec);
 
 			/* Trim the extent. */
@@ -1400,11 +1417,9 @@ xfs_swap_extent_rmap(
 		offset_fsb += ilen;
 	}
 
-	tip->i_d.di_flags2 = tip_flags2;
-	return 0;
-
 out:
-	trace_xfs_swap_extent_rmap_error(ip, error, _RET_IP_);
+	if (error)
+		trace_xfs_swap_extent_rmap_error(ip, error, _RET_IP_);
 	tip->i_d.di_flags2 = tip_flags2;
 	return error;
 }

Re: [PATCH 2/3] xfs: clean up the metadata validation in xfs_swap_extent_rmap

[Christoph Hellwig]

On Mon, May 04, 2020 at 06:10:22PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Bail out if there's something not right with either file's fork
> mappings.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/xfs_bmap_util.c |   31 +++++++++++++++++++++++--------
>  1 file changed, 23 insertions(+), 8 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index cc23a3e23e2d..2774939e176d 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -1342,8 +1342,16 @@ xfs_swap_extent_rmap(
>  				&nimaps, 0);
>  		if (error)
>  			goto out;
> -		ASSERT(nimaps == 1);
> -		ASSERT(tirec.br_startblock != DELAYSTARTBLOCK);
> +		if (nimaps != 1 || tirec.br_startblock == DELAYSTARTBLOCK) {
> +			/*
> +			 * We should never get no mapping or a delalloc extent
> +			 * since the donor file should have been flushed by the
> +			 * caller.
> +			 */
> +			ASSERT(0);
> +			error = -EINVAL;
> +			goto out;
> +		}

I'm not even sure the !nimaps case still exists.  Usually this will
return a hole extent, which we don't seem to handle here?

In general I think this code would be improved quite a bit by using
xfs_iext_lookup_extent instead of xfs_bmapi_read.

Same for the second hunk.

Re: [PATCH 2/3] xfs: clean up the metadata validation in xfs_swap_extent_rmap

[Darrick J. Wong]

On Wed, May 06, 2020 at 07:56:33AM -0700, Christoph Hellwig wrote:
> On Mon, May 04, 2020 at 06:10:22PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Bail out if there's something not right with either file's fork
> > mappings.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/xfs_bmap_util.c |   31 +++++++++++++++++++++++--------
> >  1 file changed, 23 insertions(+), 8 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> > index cc23a3e23e2d..2774939e176d 100644
> > --- a/fs/xfs/xfs_bmap_util.c
> > +++ b/fs/xfs/xfs_bmap_util.c
> > @@ -1342,8 +1342,16 @@ xfs_swap_extent_rmap(
> >  				&nimaps, 0);
> >  		if (error)
> >  			goto out;
> > -		ASSERT(nimaps == 1);
> > -		ASSERT(tirec.br_startblock != DELAYSTARTBLOCK);
> > +		if (nimaps != 1 || tirec.br_startblock == DELAYSTARTBLOCK) {
> > +			/*
> > +			 * We should never get no mapping or a delalloc extent
> > +			 * since the donor file should have been flushed by the
> > +			 * caller.
> > +			 */
> > +			ASSERT(0);
> > +			error = -EINVAL;
> > +			goto out;
> > +		}
> 
> I'm not even sure the !nimaps case still exists.  Usually this will
> return a hole extent, which we don't seem to handle here?

xfs_bmap_unmap_extent and xfs_bmap_map_extent are NOPs if you pass
them a hole.

But yeah, the !nimaps case doesn't seem to exist anymore.

> In general I think this code would be improved quite a bit by using
> xfs_iext_lookup_extent instead of xfs_bmapi_read.
> 
> Same for the second hunk.

I'd rather not make major changes to this code because my long range
plan is to replace all this with a much cleaner implementation in the
atomic file update series[1].  This patchset bandages the wtfs that I
found while writing that series, projecting that review of atomic file
updates is going to take a while...

--D

[1] https://lwn.net/ml/linux-fsdevel/158812825316.168506.932540609191384366.stgit@magnolia/

[PATCH 3/3] xfs: actually account for quota changes in xfs_swap_extents

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Currently, xfs_swap_extents neither checks for sufficient quota
reservation nor does it actually update quota counts when swapping the
extent forks.  While the primary known user of extent swapping (xfs_fsr)
is careful to ensure that the user/group/project ids of both files
match, this is not required by the kernel.  Consequently, unprivileged
userspace can cause the quota counts to be incorrect.

Fix this by updating quota counts when we swap the extents, and be sure
to make a quota reservation for the difference in blocks so that we can
bail out with EDQUOT early if needed.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_bmap.h |   13 +++--
 fs/xfs/xfs_bmap_util.c   |  125 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 134 insertions(+), 4 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
index f3259ad5c22c..463346caca46 100644
--- a/fs/xfs/libxfs/xfs_bmap.h
+++ b/fs/xfs/libxfs/xfs_bmap.h
@@ -158,6 +158,13 @@ static inline int xfs_bmapi_whichfork(int bmapi_flags)
 	{ BMAP_ATTRFORK,	"ATTR" }, \
 	{ BMAP_COWFORK,		"COW" }
 
+/* Return true if the extent is an allocated extent, written or not. */
+static inline bool xfs_bmap_is_mapped_extent(struct xfs_bmbt_irec *irec)
+{
+	return irec->br_startblock != HOLESTARTBLOCK &&
+		irec->br_startblock != DELAYSTARTBLOCK &&
+		!isnullstartblock(irec->br_startblock);
+}
 
 /*
  * Return true if the extent is a real, allocated extent, or false if it is  a
@@ -165,10 +172,8 @@ static inline int xfs_bmapi_whichfork(int bmapi_flags)
  */
 static inline bool xfs_bmap_is_real_extent(struct xfs_bmbt_irec *irec)
 {
-	return irec->br_state != XFS_EXT_UNWRITTEN &&
-		irec->br_startblock != HOLESTARTBLOCK &&
-		irec->br_startblock != DELAYSTARTBLOCK &&
-		!isnullstartblock(irec->br_startblock);
+	return xfs_bmap_is_mapped_extent(irec) &&
+	       irec->br_state != XFS_EXT_UNWRITTEN;
 }
 
 /*
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index 2774939e176d..bcac7530d1ac 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -1358,6 +1358,8 @@ xfs_swap_extent_rmap(
 
 		/* Unmap the old blocks in the source file. */
 		while (tirec.br_blockcount) {
+			int64_t		ip_delta = 0, tip_delta = 0;
+
 			ASSERT(tp->t_firstblock == NULLFSBLOCK);
 			trace_xfs_swap_extent_rmap_remap_piece(tip, &tirec);
 
@@ -1388,6 +1390,23 @@ xfs_swap_extent_rmap(
 					irec.br_blockcount);
 			trace_xfs_swap_extent_rmap_remap_piece(tip, &uirec);
 
+			/* Update quota accounting. */
+			if (xfs_bmap_is_mapped_extent(&irec)) {
+				tip_delta += irec.br_blockcount;
+				ip_delta -= irec.br_blockcount;
+			}
+			if (xfs_bmap_is_mapped_extent(&uirec)) {
+				tip_delta -= uirec.br_blockcount;
+				ip_delta += uirec.br_blockcount;
+			}
+
+			if (tip_delta)
+				xfs_trans_mod_dquot_byino(tp, tip,
+						XFS_TRANS_DQ_BCOUNT, tip_delta);
+			if (ip_delta)
+				xfs_trans_mod_dquot_byino(tp, ip,
+						XFS_TRANS_DQ_BCOUNT, ip_delta);
+
 			/* Remove the mapping from the donor file. */
 			xfs_bmap_unmap_extent(tp, tip, &uirec);
 
@@ -1435,6 +1454,7 @@ xfs_swap_extent_forks(
 {
 	xfs_filblks_t		aforkblks = 0;
 	xfs_filblks_t		taforkblks = 0;
+	int64_t			temp_blks;
 	xfs_extnum_t		junk;
 	uint64_t		tmp;
 	int			error;
@@ -1476,6 +1496,15 @@ xfs_swap_extent_forks(
 	 */
 	swap(ip->i_df, tip->i_df);
 
+	/* Update quota accounting. */
+	temp_blks = tip->i_d.di_nblocks - taforkblks + aforkblks;
+	xfs_trans_mod_dquot_byino(tp, ip, XFS_TRANS_DQ_BCOUNT,
+			temp_blks - ip->i_d.di_nblocks);
+
+	temp_blks = ip->i_d.di_nblocks + taforkblks - aforkblks;
+	xfs_trans_mod_dquot_byino(tp, tip, XFS_TRANS_DQ_BCOUNT,
+			temp_blks - tip->i_d.di_nblocks);
+
 	/*
 	 * Fix the on-disk inode values
 	 */
@@ -1566,6 +1595,93 @@ xfs_swap_change_owner(
 	return error;
 }
 
+/*
+ * Obtain a quota reservation to make sure we don't hit EDQUOT.  We can skip
+ * this if quota enforcement is disabled or if both inodes' dquots are the
+ * same.
+ */
+STATIC int
+xfs_swap_extents_prep_quota(
+	struct xfs_trans	*tp,
+	struct xfs_inode	*ip,
+	struct xfs_inode	*tip)
+{
+	struct xfs_mount	*mp = ip->i_mount;
+	xfs_filblks_t		aforkblks = 0;
+	xfs_filblks_t		taforkblks = 0;
+	xfs_filblks_t		ip_mapped, tip_mapped;
+	xfs_extnum_t		junk;
+	int			error;
+
+	/*
+	 * Don't bother with a quota reservation if we're not enforcing them
+	 * or the two inodes have the same dquots.
+	 */
+	if (!(mp->m_qflags & XFS_ALL_QUOTA_ENFD) ||
+	    (ip->i_udquot == tip->i_udquot &&
+	     ip->i_gdquot == tip->i_gdquot &&
+	     ip->i_pdquot == tip->i_pdquot))
+		return 0;
+
+	/*
+	 * Count the number of extended attribute blocks
+	 */
+	if ( ((XFS_IFORK_Q(ip) != 0) && (ip->i_d.di_anextents > 0)) &&
+	     (ip->i_d.di_aformat != XFS_DINODE_FMT_LOCAL)) {
+		error = xfs_bmap_count_blocks(tp, ip, XFS_ATTR_FORK, &junk,
+				&aforkblks);
+		if (error)
+			return error;
+	}
+	if ( ((XFS_IFORK_Q(tip) != 0) && (tip->i_d.di_anextents > 0)) &&
+	     (tip->i_d.di_aformat != XFS_DINODE_FMT_LOCAL)) {
+		error = xfs_bmap_count_blocks(tp, tip, XFS_ATTR_FORK, &junk,
+				&taforkblks);
+		if (error)
+			return error;
+	}
+
+	/* Figure out how many blocks we'll move out of each file. */
+	ip_mapped = ip->i_d.di_nblocks - aforkblks;
+	tip_mapped = tip->i_d.di_nblocks - taforkblks;
+
+	/*
+	 * For each file, compute the net gain in the number of blocks that
+	 * will be mapped into that file and reserve that much quota.  The
+	 * quota counts must be able to absorb at least that much space.
+	 */
+	if (tip_mapped > ip_mapped) {
+		error = xfs_trans_reserve_quota_nblks(tp, ip,
+				tip_mapped - ip_mapped, 0,
+				XFS_QMOPT_RES_REGBLKS);
+		if (error)
+			return error;
+	}
+
+	if (ip_mapped > tip_mapped) {
+		error = xfs_trans_reserve_quota_nblks(tp, tip,
+				ip_mapped - tip_mapped, 0,
+				XFS_QMOPT_RES_REGBLKS);
+		if (error)
+			return error;
+	}
+
+	/*
+	 * For each file, forcibly reserve the gross gain in mapped blocks so
+	 * that we don't trip over any quota block reservation assertions.
+	 * We must reserve the gross gain because the quota code subtracts from
+	 * bcount the number of blocks that we unmap; it does not add that
+	 * quantity back to the quota block reservation.
+	 */
+	error = xfs_trans_reserve_quota_nblks(tp, ip, ip_mapped, 0,
+			XFS_QMOPT_FORCE_RES | XFS_QMOPT_RES_REGBLKS);
+	if (error)
+		return error;
+
+	return xfs_trans_reserve_quota_nblks(tp, tip, tip_mapped, 0,
+			XFS_QMOPT_FORCE_RES | XFS_QMOPT_RES_REGBLKS);
+}
+
 int
 xfs_swap_extents(
 	struct xfs_inode	*ip,	/* target inode */
@@ -1702,6 +1818,15 @@ xfs_swap_extents(
 		goto out_trans_cancel;
 	}
 
+	/*
+	 * Reserve ourselves some quota if any of them are in enforcing mode.
+	 * In theory we only need enough to satisfy the change in the number
+	 * of blocks between the two ranges being remapped.
+	 */
+	error = xfs_swap_extents_prep_quota(tp, ip, tip);
+	if (error)
+		goto out_trans_cancel;
+
 	/*
 	 * Note the trickiness in setting the log flags - we set the owner log
 	 * flag on the opposite inode (i.e. the inode we are setting the new

Re: [PATCH 3/3] xfs: actually account for quota changes in xfs_swap_extents

[Christoph Hellwig]

On Mon, May 04, 2020 at 06:10:29PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Currently, xfs_swap_extents neither checks for sufficient quota
> reservation nor does it actually update quota counts when swapping the
> extent forks.  While the primary known user of extent swapping (xfs_fsr)
> is careful to ensure that the user/group/project ids of both files
> match, this is not required by the kernel.  Consequently, unprivileged
> userspace can cause the quota counts to be incorrect.

Wouldn't be the right fix to enforce an id match?  I think that is a
very sensible limitation.

Re: [PATCH 3/3] xfs: actually account for quota changes in xfs_swap_extents

[Darrick J. Wong]

On Wed, May 06, 2020 at 07:57:28AM -0700, Christoph Hellwig wrote:
> On Mon, May 04, 2020 at 06:10:29PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Currently, xfs_swap_extents neither checks for sufficient quota
> > reservation nor does it actually update quota counts when swapping the
> > extent forks.  While the primary known user of extent swapping (xfs_fsr)
> > is careful to ensure that the user/group/project ids of both files
> > match, this is not required by the kernel.  Consequently, unprivileged
> > userspace can cause the quota counts to be incorrect.
> 
> Wouldn't be the right fix to enforce an id match?  I think that is a
> very sensible limitation.

One could do that, but at a cost of breaking any userspace program that
was using XFS_IOC_SWAPEXT and was not aware that the ids had to match
(possibly due to the lack of documentation...)

It was trivial enough to port this from the atomic file update series,
so I decided to post the least functionality-reducing version and see
what happens.

Thoughts?

--D

Re: [PATCH 3/3] xfs: actually account for quota changes in xfs_swap_extents

[Christoph Hellwig]

On Wed, May 06, 2020 at 09:34:24AM -0700, Darrick J. Wong wrote:
> On Wed, May 06, 2020 at 07:57:28AM -0700, Christoph Hellwig wrote:
> > On Mon, May 04, 2020 at 06:10:29PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > 
> > > Currently, xfs_swap_extents neither checks for sufficient quota
> > > reservation nor does it actually update quota counts when swapping the
> > > extent forks.  While the primary known user of extent swapping (xfs_fsr)
> > > is careful to ensure that the user/group/project ids of both files
> > > match, this is not required by the kernel.  Consequently, unprivileged
> > > userspace can cause the quota counts to be incorrect.
> > 
> > Wouldn't be the right fix to enforce an id match?  I think that is a
> > very sensible limitation.
> 
> One could do that, but at a cost of breaking any userspace program that
> was using XFS_IOC_SWAPEXT and was not aware that the ids had to match
> (possibly due to the lack of documentation...)

I don't really expect that to be the case.  I'd throw in the check
and a printk_once warning, and I bet a beer at the next conference
(if there ever is one :)) that no one will trigger it.

Re: [PATCH 3/3] xfs: actually account for quota changes in xfs_swap_extents

[Darrick J. Wong]

On Wed, May 06, 2020 at 11:02:05PM -0700, Christoph Hellwig wrote:
> On Wed, May 06, 2020 at 09:34:24AM -0700, Darrick J. Wong wrote:
> > On Wed, May 06, 2020 at 07:57:28AM -0700, Christoph Hellwig wrote:
> > > On Mon, May 04, 2020 at 06:10:29PM -0700, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > > 
> > > > Currently, xfs_swap_extents neither checks for sufficient quota
> > > > reservation nor does it actually update quota counts when swapping the
> > > > extent forks.  While the primary known user of extent swapping (xfs_fsr)
> > > > is careful to ensure that the user/group/project ids of both files
> > > > match, this is not required by the kernel.  Consequently, unprivileged
> > > > userspace can cause the quota counts to be incorrect.
> > > 
> > > Wouldn't be the right fix to enforce an id match?  I think that is a
> > > very sensible limitation.
> > 
> > One could do that, but at a cost of breaking any userspace program that
> > was using XFS_IOC_SWAPEXT and was not aware that the ids had to match
> > (possibly due to the lack of documentation...)
> 
> I don't really expect that to be the case.  I'd throw in the check
> and a printk_once warning, and I bet a beer at the next conference
> (if there ever is one :)) that no one will trigger it.

<shrug> I guess I can at least see if fstests fails if you don't allow
swapping between extents with different [ugp]ids, but this really feels
like cutting corners off the quota functionality... :P

--D