[PATCH 0/2] xfs: random fixes

[PATCH 0/2] xfs: random fixes

[Darrick J. Wong]

Hi all,

This short series contains a couple of very short fixes that I found
while running fuzz tests and doing more audits of the rt code via
fstests.

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
---
 fs/xfs/libxfs/xfs_bmap.c |    9 ++++++---
 fs/xfs/scrub/dabtree.c   |   14 ++++++++++++++
 2 files changed, 20 insertions(+), 3 deletions(-)

[PATCH 1/2] xfs: don't free rt blocks when we're doing a REMAP bunmapi call

[Darrick J. Wong]

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

When callers pass XFS_BMAPI_REMAP into xfs_bunmapi, they want the extent
to be unmapped from the given file fork without the extent being freed.
We do this for non-rt files, but we forgot to do this for realtime
files.  So far this isn't a big deal since nobody makes a bunmapi call
to a rt file with the REMAP flag set, but don't leave a logic bomb.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_bmap.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 1b0a01b06a05..e8cd0012a017 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -5057,9 +5057,12 @@ xfs_bmap_del_extent_real(
 				  &mod);
 		ASSERT(mod == 0);
 
-		error = xfs_rtfree_extent(tp, bno, (xfs_extlen_t)len);
-		if (error)
-			goto done;
+		if (!(bflags & XFS_BMAPI_REMAP)) {
+			error = xfs_rtfree_extent(tp, bno, (xfs_extlen_t)len);
+			if (error)
+				goto done;
+		}
+
 		do_fx = 0;
 		nblks = len * mp->m_sb.sb_rextsize;
 		qfield = XFS_TRANS_DQ_RTBCOUNT;

[PATCH 2/2] xfs: check dabtree node hash values when loading child blocks

[Darrick J. Wong]

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

When xchk_da_btree_block is loading a non-root dabtree block, we know
that the parent block had to have a (hashval, address) pointer to the
block that we just loaded.  Check that the hashval in the parent matches
the block we just loaded.

This was found by fuzzing nbtree[3].hashval = ones in xfs/394.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/scrub/dabtree.c |   14 ++++++++++++++
 1 file changed, 14 insertions(+)


diff --git a/fs/xfs/scrub/dabtree.c b/fs/xfs/scrub/dabtree.c
index e56786f0a13c..653f3280e1c1 100644
--- a/fs/xfs/scrub/dabtree.c
+++ b/fs/xfs/scrub/dabtree.c
@@ -441,6 +441,20 @@ xchk_da_btree_block(
 		goto out_freebp;
 	}
 
+	/*
+	 * If we've been handed a block that is below the dabtree root, does
+	 * its hashval match what the parent block expected to see?
+	 */
+	if (level > 0) {
+		struct xfs_da_node_entry	*key;
+
+		key = xchk_da_btree_node_entry(ds, level - 1);
+		if (be32_to_cpu(key->hashval) != blk->hashval) {
+			xchk_da_set_corrupt(ds, level);
+			goto out_freebp;
+		}
+	}
+
 out:
 	return error;
 out_freebp:

Re: [PATCH 1/2] xfs: don't free rt blocks when we're doing a REMAP bunmapi call

[Dave Chinner]

On Wed, Sep 16, 2020 at 08:28:33PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> When callers pass XFS_BMAPI_REMAP into xfs_bunmapi, they want the extent
> to be unmapped from the given file fork without the extent being freed.
> We do this for non-rt files, but we forgot to do this for realtime
> files.  So far this isn't a big deal since nobody makes a bunmapi call
> to a rt file with the REMAP flag set, but don't leave a logic bomb.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_bmap.c |    9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)

Reasonable.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/2] xfs: check dabtree node hash values when loading child blocks

[Dave Chinner]

On Wed, Sep 16, 2020 at 08:28:39PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> When xchk_da_btree_block is loading a non-root dabtree block, we know
> that the parent block had to have a (hashval, address) pointer to the
> block that we just loaded.  Check that the hashval in the parent matches
> the block we just loaded.
> 
> This was found by fuzzing nbtree[3].hashval = ones in xfs/394.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/scrub/dabtree.c |   14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> 
> diff --git a/fs/xfs/scrub/dabtree.c b/fs/xfs/scrub/dabtree.c
> index e56786f0a13c..653f3280e1c1 100644
> --- a/fs/xfs/scrub/dabtree.c
> +++ b/fs/xfs/scrub/dabtree.c
> @@ -441,6 +441,20 @@ xchk_da_btree_block(
>  		goto out_freebp;
>  	}
>  
> +	/*
> +	 * If we've been handed a block that is below the dabtree root, does
> +	 * its hashval match what the parent block expected to see?
> +	 */
> +	if (level > 0) {
> +		struct xfs_da_node_entry	*key;
> +
> +		key = xchk_da_btree_node_entry(ds, level - 1);
> +		if (be32_to_cpu(key->hashval) != blk->hashval) {
> +			xchk_da_set_corrupt(ds, level);
> +			goto out_freebp;
> +		}
> +	}

Looks ok.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/2] xfs: don't free rt blocks when we're doing a REMAP bunmapi call

[Christoph Hellwig]

On Wed, Sep 16, 2020 at 08:28:33PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> When callers pass XFS_BMAPI_REMAP into xfs_bunmapi, they want the extent
> to be unmapped from the given file fork without the extent being freed.
> We do this for non-rt files, but we forgot to do this for realtime
> files.  So far this isn't a big deal since nobody makes a bunmapi call
> to a rt file with the REMAP flag set, but don't leave a logic bomb.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_bmap.c |    9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 1b0a01b06a05..e8cd0012a017 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -5057,9 +5057,12 @@ xfs_bmap_del_extent_real(
>  				  &mod);
>  		ASSERT(mod == 0);
>  
> -		error = xfs_rtfree_extent(tp, bno, (xfs_extlen_t)len);
> -		if (error)
> -			goto done;
> +		if (!(bflags & XFS_BMAPI_REMAP)) {
> +			error = xfs_rtfree_extent(tp, bno, (xfs_extlen_t)len);
> +			if (error)
> +				goto done;
> +		}
> +
>  		do_fx = 0;
>  		nblks = len * mp->m_sb.sb_rextsize;
>  		qfield = XFS_TRANS_DQ_RTBCOUNT;

We also don't need to calculate bno for this case.

Re: [PATCH 2/2] xfs: check dabtree node hash values when loading child blocks

[Christoph Hellwig]

On Wed, Sep 16, 2020 at 08:28:39PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> When xchk_da_btree_block is loading a non-root dabtree block, we know
> that the parent block had to have a (hashval, address) pointer to the
> block that we just loaded.  Check that the hashval in the parent matches
> the block we just loaded.
> 
> This was found by fuzzing nbtree[3].hashval = ones in xfs/394.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

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

Re: [PATCH 1/2] xfs: don't free rt blocks when we're doing a REMAP bunmapi call

[Darrick J. Wong]

On Thu, Sep 17, 2020 at 09:11:34AM +0100, Christoph Hellwig wrote:
> On Wed, Sep 16, 2020 at 08:28:33PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > When callers pass XFS_BMAPI_REMAP into xfs_bunmapi, they want the extent
> > to be unmapped from the given file fork without the extent being freed.
> > We do this for non-rt files, but we forgot to do this for realtime
> > files.  So far this isn't a big deal since nobody makes a bunmapi call
> > to a rt file with the REMAP flag set, but don't leave a logic bomb.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/libxfs/xfs_bmap.c |    9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> > index 1b0a01b06a05..e8cd0012a017 100644
> > --- a/fs/xfs/libxfs/xfs_bmap.c
> > +++ b/fs/xfs/libxfs/xfs_bmap.c
> > @@ -5057,9 +5057,12 @@ xfs_bmap_del_extent_real(
> >  				  &mod);
> >  		ASSERT(mod == 0);
> >  
> > -		error = xfs_rtfree_extent(tp, bno, (xfs_extlen_t)len);
> > -		if (error)
> > -			goto done;
> > +		if (!(bflags & XFS_BMAPI_REMAP)) {
> > +			error = xfs_rtfree_extent(tp, bno, (xfs_extlen_t)len);
> > +			if (error)
> > +				goto done;
> > +		}
> > +
> >  		do_fx = 0;
> >  		nblks = len * mp->m_sb.sb_rextsize;
> >  		qfield = XFS_TRANS_DQ_RTBCOUNT;
> 
> We also don't need to calculate bno for this case.

Fixed.

--D

[PATCH v2 1/2] xfs: don't free rt blocks when we're doing a REMAP bunmapi call

[Darrick J. Wong]

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

When callers pass XFS_BMAPI_REMAP into xfs_bunmapi, they want the extent
to be unmapped from the given file fork without the extent being freed.
We do this for non-rt files, but we forgot to do this for realtime
files.  So far this isn't a big deal since nobody makes a bunmapi call
to a rt file with the REMAP flag set, but don't leave a logic bomb.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
v2: only compute bno if we're going to use it
---
 fs/xfs/libxfs/xfs_bmap.c |   19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 1b0a01b06a05..d9a692484eae 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -5046,20 +5046,25 @@ xfs_bmap_del_extent_real(
 
 	flags = XFS_ILOG_CORE;
 	if (whichfork == XFS_DATA_FORK && XFS_IS_REALTIME_INODE(ip)) {
-		xfs_fsblock_t	bno;
 		xfs_filblks_t	len;
 		xfs_extlen_t	mod;
 
-		bno = div_u64_rem(del->br_startblock, mp->m_sb.sb_rextsize,
-				  &mod);
-		ASSERT(mod == 0);
 		len = div_u64_rem(del->br_blockcount, mp->m_sb.sb_rextsize,
 				  &mod);
 		ASSERT(mod == 0);
 
-		error = xfs_rtfree_extent(tp, bno, (xfs_extlen_t)len);
-		if (error)
-			goto done;
+		if (!(bflags & XFS_BMAPI_REMAP)) {
+			xfs_fsblock_t	bno;
+
+			bno = div_u64_rem(del->br_startblock,
+					mp->m_sb.sb_rextsize, &mod);
+			ASSERT(mod == 0);
+
+			error = xfs_rtfree_extent(tp, bno, (xfs_extlen_t)len);
+			if (error)
+				goto done;
+		}
+
 		do_fx = 0;
 		nblks = len * mp->m_sb.sb_rextsize;
 		qfield = XFS_TRANS_DQ_RTBCOUNT;

Re: [PATCH v2 1/2] xfs: don't free rt blocks when we're doing a REMAP bunmapi call

[Christoph Hellwig]

On Thu, Sep 17, 2020 at 07:14:50PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> When callers pass XFS_BMAPI_REMAP into xfs_bunmapi, they want the extent
> to be unmapped from the given file fork without the extent being freed.
> We do this for non-rt files, but we forgot to do this for realtime
> files.  So far this isn't a big deal since nobody makes a bunmapi call
> to a rt file with the REMAP flag set, but don't leave a logic bomb.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

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

Re: [PATCH v2 1/2] xfs: don't free rt blocks when we're doing a REMAP bunmapi call

[Dave Chinner]

On Thu, Sep 17, 2020 at 07:14:50PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> When callers pass XFS_BMAPI_REMAP into xfs_bunmapi, they want the extent
> to be unmapped from the given file fork without the extent being freed.
> We do this for non-rt files, but we forgot to do this for realtime
> files.  So far this isn't a big deal since nobody makes a bunmapi call
> to a rt file with the REMAP flag set, but don't leave a logic bomb.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> v2: only compute bno if we're going to use it
> ---
>  fs/xfs/libxfs/xfs_bmap.c |   19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)

Looks fine.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com