[PATCH 00/10] xfs: fix various checking problems

[PATCH 00/10] xfs: fix various checking problems

[Darrick J. Wong]

Hi all,

Here's a somewhat longer series of patches to fix some bugs and other
inadequacies in the metadata integrity checking of XFS.  The first patch
makes it so that we can set the dax file flag on a directory so that the
daxiness propagates to new files even if the data device itself does not
support dax.

The rest of the series replaces various ASSERTs in the btree and
directory handling code with error logging and EFSCORRUPTED returns so
that maliciously corrupted filesystem images error out immediately (now
that ASSERT is no longer always fatal).  All of these fixes were found
via online scrub fuzz tests.

Comments and questions are, as always, welcome.

--D

[PATCH 01/10] xfs: don't forbid setting dax flag on directories if device doesn't dax

[Darrick J. Wong]

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

On a directory, the DAX flag is merely a hint that files created in the
directory should have the DAX flag set at creation time.  We don't care
if the underlying device supports DAX or not because directory metadata
are always cached in DRAM.  We don't care if new files get the flag even
if the device doesn't support DAX because we always check for DAX
support before setting the VFS flag (S_DAX).

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


diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 5dd9e22b4a4c..82f7c83c1dad 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1098,12 +1098,14 @@ xfs_ioctl_setattr_dax_invalidate(
 	/*
 	 * It is only valid to set the DAX flag on regular files and
 	 * directories on filesystems where the block size is equal to the page
-	 * size. On directories it serves as an inherit hint.
+	 * size. On directories it serves as an inherited hint so we don't
+	 * have to check the device for dax support or flush pagecache.
 	 */
 	if (fa->fsx_xflags & FS_XFLAG_DAX) {
 		if (!(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode)))
 			return -EINVAL;
-		if (!bdev_dax_supported(xfs_find_bdev_for_inode(VFS_I(ip)),
+		if (S_ISREG(inode->i_mode) &&
+		    !bdev_dax_supported(xfs_find_bdev_for_inode(VFS_I(ip)),
 				sb->s_blocksize))
 			return -EINVAL;
 	}
@@ -1114,6 +1116,9 @@ xfs_ioctl_setattr_dax_invalidate(
 	if (!(fa->fsx_xflags & FS_XFLAG_DAX) && !IS_DAX(inode))
 		return 0;
 
+	if (S_ISDIR(inode->i_mode))
+		return 0;
+
 	/* lock, flush and invalidate mapping in preparation for flag change */
 	xfs_ilock(ip, XFS_MMAPLOCK_EXCL | XFS_IOLOCK_EXCL);
 	error = filemap_write_and_wait(inode->i_mapping);

Re: [PATCH 01/10] xfs: don't forbid setting dax flag on directories if device doesn't dax

[Dave Chinner]

On Sun, Jun 03, 2018 at 04:22:28PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> On a directory, the DAX flag is merely a hint that files created in the
> directory should have the DAX flag set at creation time.  We don't care
> if the underlying device supports DAX or not because directory metadata
> are always cached in DRAM.  We don't care if new files get the flag even
> if the device doesn't support DAX because we always check for DAX
> support before setting the VFS flag (S_DAX).
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Am I correct in assuming this allows directory hints to work on
a filesystem with a DAX capable RT device, but a normal data device?

Regardless, behaviour seems sensible to me.

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

Re: [PATCH 01/10] xfs: don't forbid setting dax flag on directories if device doesn't dax

[Darrick J. Wong]

On Mon, Jun 04, 2018 at 09:41:36AM +1000, Dave Chinner wrote:
> On Sun, Jun 03, 2018 at 04:22:28PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > On a directory, the DAX flag is merely a hint that files created in the
> > directory should have the DAX flag set at creation time.  We don't care
> > if the underlying device supports DAX or not because directory metadata
> > are always cached in DRAM.  We don't care if new files get the flag even
> > if the device doesn't support DAX because we always check for DAX
> > support before setting the VFS flag (S_DAX).
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Am I correct in assuming this allows directory hints to work on
> a filesystem with a DAX capable RT device, but a normal data device?

Correct.

--D

> Regardless, behaviour seems sensible to me.
> 
> Reviewed-by: Dave Chinner <dchinner@redhat.com>
> -- 
> Dave Chinner
> david@fromorbit.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 02/10] xfs: strengthen btree pointer checks before use

[Darrick J. Wong]

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

Instead of ASSERTing on null btree pointers in xfs_btree_ptr_to_daddr,
use the new block number verifiers to ensure that the btree pointer
doesn't point to any sensitive areas (AG headers, past-EOFS) and return
-EFSCORRUPTED if this is the case.  Remove the ASSERT because on-disk
corruptions shouldn't trigger ASSERTs.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_btree.c |   60 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 47 insertions(+), 13 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
index c825c8182b30..2ddf47f3be52 100644
--- a/fs/xfs/libxfs/xfs_btree.c
+++ b/fs/xfs/libxfs/xfs_btree.c
@@ -988,22 +988,44 @@ xfs_btree_readahead(
 	return xfs_btree_readahead_sblock(cur, lr, block);
 }
 
-STATIC xfs_daddr_t
+STATIC int
 xfs_btree_ptr_to_daddr(
 	struct xfs_btree_cur	*cur,
-	union xfs_btree_ptr	*ptr)
+	union xfs_btree_ptr	*ptr,
+	xfs_daddr_t		*daddr)
 {
+	xfs_fsblock_t		fsbno;
+	xfs_agblock_t		agbno;
+
 	if (cur->bc_flags & XFS_BTREE_LONG_PTRS) {
-		ASSERT(ptr->l != cpu_to_be64(NULLFSBLOCK));
+		fsbno = be64_to_cpu(ptr->l);
+
+		if (!xfs_btree_check_lptr(cur, fsbno, 1)) {
+			xfs_err(cur->bc_mp,
+"Inode %llu fork %d: Corrupt btree %d pointer to fsblock %llu",
+					cur->bc_private.b.ip->i_ino,
+					cur->bc_private.b.whichfork,
+					cur->bc_btnum, fsbno);
+			return -EFSCORRUPTED;
+		}
 
-		return XFS_FSB_TO_DADDR(cur->bc_mp, be64_to_cpu(ptr->l));
+		*daddr = XFS_FSB_TO_DADDR(cur->bc_mp, fsbno);
 	} else {
-		ASSERT(cur->bc_private.a.agno != NULLAGNUMBER);
-		ASSERT(ptr->s != cpu_to_be32(NULLAGBLOCK));
+		agbno = be32_to_cpu(ptr->s);
+
+		if (!xfs_btree_check_sptr(cur, agbno, 1)) {
+			xfs_err(cur->bc_mp,
+"AG %u: Corrupt btree %d pointer to agbno %u",
+					cur->bc_private.a.agno, cur->bc_btnum,
+					agbno);
+			return -EFSCORRUPTED;
+		}
 
-		return XFS_AGB_TO_DADDR(cur->bc_mp, cur->bc_private.a.agno,
-					be32_to_cpu(ptr->s));
+		*daddr = XFS_AGB_TO_DADDR(cur->bc_mp, cur->bc_private.a.agno,
+					agbno);
 	}
+
+	return 0;
 }
 
 /*
@@ -1018,8 +1040,11 @@ xfs_btree_readahead_ptr(
 	union xfs_btree_ptr	*ptr,
 	xfs_extlen_t		count)
 {
-	xfs_buf_readahead(cur->bc_mp->m_ddev_targp,
-			  xfs_btree_ptr_to_daddr(cur, ptr),
+	xfs_daddr_t		daddr;
+
+	if (xfs_btree_ptr_to_daddr(cur, ptr, &daddr))
+		return;
+	xfs_buf_readahead(cur->bc_mp->m_ddev_targp, daddr,
 			  cur->bc_mp->m_bsize * count, cur->bc_ops->buf_ops);
 }
 
@@ -1282,11 +1307,14 @@ xfs_btree_get_buf_block(
 {
 	struct xfs_mount	*mp = cur->bc_mp;
 	xfs_daddr_t		d;
+	int			error;
 
 	/* need to sort out how callers deal with failures first */
 	ASSERT(!(flags & XBF_TRYLOCK));
 
-	d = xfs_btree_ptr_to_daddr(cur, ptr);
+	error = xfs_btree_ptr_to_daddr(cur, ptr, &d);
+	if (error)
+		return error;
 	*bpp = xfs_trans_get_buf(cur->bc_tp, mp->m_ddev_targp, d,
 				 mp->m_bsize, flags);
 
@@ -1317,7 +1345,9 @@ xfs_btree_read_buf_block(
 	/* need to sort out how callers deal with failures first */
 	ASSERT(!(flags & XBF_TRYLOCK));
 
-	d = xfs_btree_ptr_to_daddr(cur, ptr);
+	error = xfs_btree_ptr_to_daddr(cur, ptr, &d);
+	if (error)
+		return error;
 	error = xfs_trans_read_buf(mp, cur->bc_tp, mp->m_ddev_targp, d,
 				   mp->m_bsize, flags, bpp,
 				   cur->bc_ops->buf_ops);
@@ -1764,6 +1794,7 @@ xfs_btree_lookup_get_block(
 	struct xfs_btree_block	**blkp) /* return btree block */
 {
 	struct xfs_buf		*bp;	/* buffer pointer for btree block */
+	xfs_daddr_t		daddr;
 	int			error = 0;
 
 	/* special case the root block if in an inode */
@@ -1780,7 +1811,10 @@ xfs_btree_lookup_get_block(
 	 * Otherwise throw it away and get a new one.
 	 */
 	bp = cur->bc_bufs[level];
-	if (bp && XFS_BUF_ADDR(bp) == xfs_btree_ptr_to_daddr(cur, pp)) {
+	error = xfs_btree_ptr_to_daddr(cur, pp, &daddr);
+	if (error)
+		return error;
+	if (bp && XFS_BUF_ADDR(bp) == daddr) {
 		*blkp = XFS_BUF_TO_BLOCK(bp);
 		return 0;
 	}

Re: [PATCH 02/10] xfs: strengthen btree pointer checks before use

[Dave Chinner]

On Sun, Jun 03, 2018 at 04:22:34PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Instead of ASSERTing on null btree pointers in xfs_btree_ptr_to_daddr,
> use the new block number verifiers to ensure that the btree pointer
> doesn't point to any sensitive areas (AG headers, past-EOFS) and return
> -EFSCORRUPTED if this is the case.  Remove the ASSERT because on-disk
> corruptions shouldn't trigger ASSERTs.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good.

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

[PATCH 03/10] xfs: don't assert when on-disk btree pointers are garbage

[Darrick J. Wong]

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

Don't ASSERT when we encounter bad on-disk btree pointers in the debug
check functions.  Log the error to leave breadcrumbs and let the upper
layers deal with it.

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


diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
index 2ddf47f3be52..6101ad685019 100644
--- a/fs/xfs/libxfs/xfs_btree.c
+++ b/fs/xfs/libxfs/xfs_btree.c
@@ -247,16 +247,25 @@ xfs_btree_check_ptr(
 	int			level)
 {
 	if (cur->bc_flags & XFS_BTREE_LONG_PTRS) {
-		XFS_WANT_CORRUPTED_RETURN(cur->bc_mp,
-				xfs_btree_check_lptr(cur,
-					be64_to_cpu((&ptr->l)[index]), level));
+		if (xfs_btree_check_lptr(cur, be64_to_cpu((&ptr->l)[index]),
+				level))
+			return 0;
+		xfs_err(cur->bc_mp,
+"Inode %llu fork %d: Corrupt btree %d pointer at level %d index %d.",
+				cur->bc_private.b.ip->i_ino,
+				cur->bc_private.b.whichfork, cur->bc_btnum,
+				level, index);
 	} else {
-		XFS_WANT_CORRUPTED_RETURN(cur->bc_mp,
-				xfs_btree_check_sptr(cur,
-					be32_to_cpu((&ptr->s)[index]), level));
+		if (xfs_btree_check_sptr(cur, be32_to_cpu((&ptr->s)[index]),
+				level))
+			return 0;
+		xfs_err(cur->bc_mp,
+"AG %u: Corrupt btree %d pointer at level %d index %d.",
+				cur->bc_private.a.agno, cur->bc_btnum,
+				level, index);
 	}
 
-	return 0;
+	return -EFSCORRUPTED;
 }
 #endif
 

Re: [PATCH 03/10] xfs: don't assert when on-disk btree pointers are garbage

[Dave Chinner]

On Sun, Jun 03, 2018 at 04:22:41PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Don't ASSERT when we encounter bad on-disk btree pointers in the debug
> check functions.  Log the error to leave breadcrumbs and let the upper
> layers deal with it.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_btree.c |   23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)

Isn't this the same checks that were in the last patch? Can
you lift xfs_btree_check_ptr() out of the debug only code, and
make the previous patch call it?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 03/10] xfs: don't assert when on-disk btree pointers are garbage

[Dave Chinner]

On Mon, Jun 04, 2018 at 09:49:30AM +1000, Dave Chinner wrote:
> On Sun, Jun 03, 2018 at 04:22:41PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Don't ASSERT when we encounter bad on-disk btree pointers in the debug
> > check functions.  Log the error to leave breadcrumbs and let the upper
> > layers deal with it.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/libxfs/xfs_btree.c |   23 ++++++++++++++++-------
> >  1 file changed, 16 insertions(+), 7 deletions(-)
> 
> Isn't this the same checks that were in the last patch? Can
> you lift xfs_btree_check_ptr() out of the debug only code, and
> make the previous patch call it?

Another thought on this - we can get rid of all the ifdef DEBUG
wrappers around xfs_btree_check_ptr() for debug kernels by adding:

#ifdef DEBUG
#define xfs_btree_debug_check_ptr	xfs_btree_check_ptr
else
#define xfs_btree_debug_check_ptr(...)	0
#endif

or do something like xfs_dir3_data_check/__xfs_dir3_data_check().
That will clean the core btree code up a fair bit...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

[PATCH 04/10] xfs: check directory bestfree information in the verifier

[Darrick J. Wong]

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

Create a variant of xfs_dir2_data_freefind that is suitable for use in a
verifier.  Because _freefind is called by the verifier, we simply
duplicate the _freefind function, convert the ASSERTs to return
__this_address, and modify the verifier to call our new function.  Once
we've made it impossible for directory blocks with bad bestfree data to
make it into the filesystem we can remove the DEBUG code from the
regular _freefind function.

Underlying argument: corruption of on-disk metadata should return
-EFSCORRUPTED instead of blowing ASSERTs.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_dir2_data.c |  121 +++++++++++++++++++++++++++++------------
 1 file changed, 86 insertions(+), 35 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_dir2_data.c b/fs/xfs/libxfs/xfs_dir2_data.c
index cb67ec730b9b..bc5c0ba46ec6 100644
--- a/fs/xfs/libxfs/xfs_dir2_data.c
+++ b/fs/xfs/libxfs/xfs_dir2_data.c
@@ -33,6 +33,11 @@
 #include "xfs_cksum.h"
 #include "xfs_log.h"
 
+static xfs_failaddr_t xfs_dir2_data_freefind_verify(
+		struct xfs_dir2_data_hdr *hdr, struct xfs_dir2_data_free *bf,
+		struct xfs_dir2_data_unused *dup,
+		struct xfs_dir2_data_free **bf_ent);
+
 /*
  * Check the consistency of the data block.
  * The input can also be a block-format directory.
@@ -52,6 +57,7 @@ __xfs_dir3_data_check(
 	xfs_dir2_data_free_t	*dfp;		/* bestfree entry */
 	xfs_dir2_data_unused_t	*dup;		/* unused entry */
 	char			*endp;		/* end of useful data */
+	xfs_failaddr_t		fa;
 	int			freeseen;	/* mask of bestfrees seen */
 	xfs_dahash_t		hash;		/* hash of current name */
 	int			i;		/* leaf index */
@@ -154,7 +160,9 @@ __xfs_dir3_data_check(
 			if (be16_to_cpu(*xfs_dir2_data_unused_tag_p(dup)) !=
 			    (char *)dup - (char *)hdr)
 				return __this_address;
-			dfp = xfs_dir2_data_freefind(hdr, bf, dup);
+			fa = xfs_dir2_data_freefind_verify(hdr, bf, dup, &dfp);
+			if (fa)
+				return fa;
 			if (dfp) {
 				i = (int)(dfp - bf);
 				if ((freeseen & (1 << i)) != 0)
@@ -381,55 +389,98 @@ xfs_dir3_data_readahead(
 }
 
 /*
- * Given a data block and an unused entry from that block,
- * return the bestfree entry if any that corresponds to it.
+ * Find the bestfree entry that exactly coincides with unused directory space
+ * or a verifier error because the bestfree data are bad.
  */
-xfs_dir2_data_free_t *
-xfs_dir2_data_freefind(
-	struct xfs_dir2_data_hdr *hdr,		/* data block header */
-	struct xfs_dir2_data_free *bf,		/* bestfree table pointer */
-	struct xfs_dir2_data_unused *dup)	/* unused space */
+static xfs_failaddr_t
+xfs_dir2_data_freefind_verify(
+	struct xfs_dir2_data_hdr	*hdr,
+	struct xfs_dir2_data_free	*bf,
+	struct xfs_dir2_data_unused	*dup,
+	struct xfs_dir2_data_free	**bf_ent)
 {
-	xfs_dir2_data_free_t	*dfp;		/* bestfree entry */
-	xfs_dir2_data_aoff_t	off;		/* offset value needed */
-#ifdef DEBUG
-	int			matched;	/* matched the value */
-	int			seenzero;	/* saw a 0 bestfree entry */
-#endif
+	struct xfs_dir2_data_free	*dfp;
+	xfs_dir2_data_aoff_t		off;
+	bool				matched = false;
+	bool				seenzero = false;
 
+	*bf_ent = NULL;
 	off = (xfs_dir2_data_aoff_t)((char *)dup - (char *)hdr);
 
-#ifdef DEBUG
 	/*
 	 * Validate some consistency in the bestfree table.
 	 * Check order, non-overlapping entries, and if we find the
 	 * one we're looking for it has to be exact.
 	 */
-	ASSERT(hdr->magic == cpu_to_be32(XFS_DIR2_DATA_MAGIC) ||
-	       hdr->magic == cpu_to_be32(XFS_DIR3_DATA_MAGIC) ||
-	       hdr->magic == cpu_to_be32(XFS_DIR2_BLOCK_MAGIC) ||
-	       hdr->magic == cpu_to_be32(XFS_DIR3_BLOCK_MAGIC));
-	for (dfp = &bf[0], seenzero = matched = 0;
-	     dfp < &bf[XFS_DIR2_DATA_FD_COUNT];
-	     dfp++) {
+	for (dfp = &bf[0]; dfp < &bf[XFS_DIR2_DATA_FD_COUNT]; dfp++) {
 		if (!dfp->offset) {
-			ASSERT(!dfp->length);
-			seenzero = 1;
+			if (dfp->length)
+				return __this_address;
+			seenzero = true;
 			continue;
 		}
-		ASSERT(seenzero == 0);
+		if (seenzero)
+			return __this_address;
 		if (be16_to_cpu(dfp->offset) == off) {
-			matched = 1;
-			ASSERT(dfp->length == dup->length);
-		} else if (off < be16_to_cpu(dfp->offset))
-			ASSERT(off + be16_to_cpu(dup->length) <= be16_to_cpu(dfp->offset));
-		else
-			ASSERT(be16_to_cpu(dfp->offset) + be16_to_cpu(dfp->length) <= off);
-		ASSERT(matched || be16_to_cpu(dfp->length) >= be16_to_cpu(dup->length));
-		if (dfp > &bf[0])
-			ASSERT(be16_to_cpu(dfp[-1].length) >= be16_to_cpu(dfp[0].length));
+			matched = true;
+			if (dfp->length != dup->length)
+				return __this_address;
+		} else if (be16_to_cpu(dfp->offset) > off) {
+			if (off + be16_to_cpu(dup->length) >
+			    be16_to_cpu(dfp->offset))
+				return __this_address;
+		} else {
+			if (be16_to_cpu(dfp->offset) +
+			    be16_to_cpu(dfp->length) > off)
+				return __this_address;
+		}
+		if (!matched &&
+		    be16_to_cpu(dfp->length) < be16_to_cpu(dup->length))
+			return __this_address;
+		if (dfp > &bf[0] &&
+		    be16_to_cpu(dfp[-1].length) < be16_to_cpu(dfp[0].length))
+			return __this_address;
 	}
-#endif
+
+	/*
+	 * If this is smaller than the smallest bestfree entry,
+	 * it can't be there since they're sorted.
+	 */
+	if (be16_to_cpu(dup->length) <
+	    be16_to_cpu(bf[XFS_DIR2_DATA_FD_COUNT - 1].length))
+		return NULL;
+	/*
+	 * Look at the three bestfree entries for our guy.
+	 */
+	for (dfp = &bf[0]; dfp < &bf[XFS_DIR2_DATA_FD_COUNT]; dfp++) {
+		if (!dfp->offset)
+			return NULL;
+		if (be16_to_cpu(dfp->offset) == off) {
+			*bf_ent = dfp;
+			return NULL;
+		}
+	}
+	/*
+	 * Didn't find it.  This only happens if there are duplicate lengths.
+	 */
+	return NULL;
+}
+
+/*
+ * Given a data block and an unused entry from that block,
+ * return the bestfree entry if any that corresponds to it.
+ */
+xfs_dir2_data_free_t *
+xfs_dir2_data_freefind(
+	struct xfs_dir2_data_hdr *hdr,		/* data block header */
+	struct xfs_dir2_data_free *bf,		/* bestfree table pointer */
+	struct xfs_dir2_data_unused *dup)	/* unused space */
+{
+	xfs_dir2_data_free_t	*dfp;		/* bestfree entry */
+	xfs_dir2_data_aoff_t	off;		/* offset value needed */
+
+	off = (xfs_dir2_data_aoff_t)((char *)dup - (char *)hdr);
+
 	/*
 	 * If this is smaller than the smallest bestfree entry,
 	 * it can't be there since they're sorted.

Re: [PATCH 04/10] xfs: check directory bestfree information in the verifier

[Dave Chinner]

On Sun, Jun 03, 2018 at 04:22:49PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Create a variant of xfs_dir2_data_freefind that is suitable for use in a
> verifier.  Because _freefind is called by the verifier, we simply
> duplicate the _freefind function, convert the ASSERTs to return
> __this_address, and modify the verifier to call our new function.  Once
> we've made it impossible for directory blocks with bad bestfree data to
> make it into the filesystem we can remove the DEBUG code from the
> regular _freefind function.
> 
> Underlying argument: corruption of on-disk metadata should return
> -EFSCORRUPTED instead of blowing ASSERTs.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_dir2_data.c |  121 +++++++++++++++++++++++++++++------------
>  1 file changed, 86 insertions(+), 35 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_dir2_data.c b/fs/xfs/libxfs/xfs_dir2_data.c
> index cb67ec730b9b..bc5c0ba46ec6 100644
> --- a/fs/xfs/libxfs/xfs_dir2_data.c
> +++ b/fs/xfs/libxfs/xfs_dir2_data.c
> @@ -33,6 +33,11 @@
>  #include "xfs_cksum.h"
>  #include "xfs_log.h"
>  
> +static xfs_failaddr_t xfs_dir2_data_freefind_verify(
> +		struct xfs_dir2_data_hdr *hdr, struct xfs_dir2_data_free *bf,
> +		struct xfs_dir2_data_unused *dup,
> +		struct xfs_dir2_data_free **bf_ent);
> +
>  /*
>   * Check the consistency of the data block.
>   * The input can also be a block-format directory.
> @@ -52,6 +57,7 @@ __xfs_dir3_data_check(
>  	xfs_dir2_data_free_t	*dfp;		/* bestfree entry */
>  	xfs_dir2_data_unused_t	*dup;		/* unused entry */
>  	char			*endp;		/* end of useful data */
> +	xfs_failaddr_t		fa;
>  	int			freeseen;	/* mask of bestfrees seen */
>  	xfs_dahash_t		hash;		/* hash of current name */
>  	int			i;		/* leaf index */

This could be placed inside the loop scope, right?

> -	ASSERT(hdr->magic == cpu_to_be32(XFS_DIR2_DATA_MAGIC) ||
> -	       hdr->magic == cpu_to_be32(XFS_DIR3_DATA_MAGIC) ||
> -	       hdr->magic == cpu_to_be32(XFS_DIR2_BLOCK_MAGIC) ||
> -	       hdr->magic == cpu_to_be32(XFS_DIR3_BLOCK_MAGIC));
> -	for (dfp = &bf[0], seenzero = matched = 0;
> -	     dfp < &bf[XFS_DIR2_DATA_FD_COUNT];
> -	     dfp++) {
> +	for (dfp = &bf[0]; dfp < &bf[XFS_DIR2_DATA_FD_COUNT]; dfp++) {
>  		if (!dfp->offset) {
> -			ASSERT(!dfp->length);
> -			seenzero = 1;
> +			if (dfp->length)
> +				return __this_address;
> +			seenzero = true;
>  			continue;
>  		}
> -		ASSERT(seenzero == 0);
> +		if (seenzero)
> +			return __this_address;
>  		if (be16_to_cpu(dfp->offset) == off) {
> -			matched = 1;
> -			ASSERT(dfp->length == dup->length);
> -		} else if (off < be16_to_cpu(dfp->offset))
> -			ASSERT(off + be16_to_cpu(dup->length) <= be16_to_cpu(dfp->offset));
> -		else
> -			ASSERT(be16_to_cpu(dfp->offset) + be16_to_cpu(dfp->length) <= off);
> -		ASSERT(matched || be16_to_cpu(dfp->length) >= be16_to_cpu(dup->length));
> -		if (dfp > &bf[0])
> -			ASSERT(be16_to_cpu(dfp[-1].length) >= be16_to_cpu(dfp[0].length));
> +			matched = true;
> +			if (dfp->length != dup->length)
> +				return __this_address;
> +		} else if (be16_to_cpu(dfp->offset) > off) {
> +			if (off + be16_to_cpu(dup->length) >
> +			    be16_to_cpu(dfp->offset))

can you indent the second line further to indicate it is a
continuation of the logic statement on the previous line rather than
a new logic condition? i.e.

			if (off + be16_to_cpu(dup->length) >
					be16_to_cpu(dfp->offset))
> +				return __this_address;
> +		} else {
> +			if (be16_to_cpu(dfp->offset) +
> +			    be16_to_cpu(dfp->length) > off)
> +				return __this_address;

Same here?

> +		}
> +		if (!matched &&
> +		    be16_to_cpu(dfp->length) < be16_to_cpu(dup->length))
> +			return __this_address;
> +		if (dfp > &bf[0] &&
> +		    be16_to_cpu(dfp[-1].length) < be16_to_cpu(dfp[0].length))
> +			return __this_address;
>  	}
> -#endif
> +
> +	/*
> +	 * If this is smaller than the smallest bestfree entry,
> +	 * it can't be there since they're sorted.
> +	 */
> +	if (be16_to_cpu(dup->length) <
> +	    be16_to_cpu(bf[XFS_DIR2_DATA_FD_COUNT - 1].length))
> +		return NULL;
> +	/*
> +	 * Look at the three bestfree entries for our guy.
> +	 */
> +	for (dfp = &bf[0]; dfp < &bf[XFS_DIR2_DATA_FD_COUNT]; dfp++) {
> +		if (!dfp->offset)
> +			return NULL;
> +		if (be16_to_cpu(dfp->offset) == off) {
> +			*bf_ent = dfp;
> +			return NULL;
> +		}
> +	}
> +	/*
> +	 * Didn't find it.  This only happens if there are duplicate lengths.
> +	 */
> +	return NULL;

And this tail is basically a duplicate of what now remains in
xfs_dir2_data_freefind(). Can you call that function rather than
duplicating the search code?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 04/10] xfs: check directory bestfree information in the verifier

[Darrick J. Wong]

On Mon, Jun 04, 2018 at 10:10:19AM +1000, Dave Chinner wrote:
> On Sun, Jun 03, 2018 at 04:22:49PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Create a variant of xfs_dir2_data_freefind that is suitable for use in a
> > verifier.  Because _freefind is called by the verifier, we simply
> > duplicate the _freefind function, convert the ASSERTs to return
> > __this_address, and modify the verifier to call our new function.  Once
> > we've made it impossible for directory blocks with bad bestfree data to
> > make it into the filesystem we can remove the DEBUG code from the
> > regular _freefind function.
> > 
> > Underlying argument: corruption of on-disk metadata should return
> > -EFSCORRUPTED instead of blowing ASSERTs.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/libxfs/xfs_dir2_data.c |  121 +++++++++++++++++++++++++++++------------
> >  1 file changed, 86 insertions(+), 35 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/libxfs/xfs_dir2_data.c b/fs/xfs/libxfs/xfs_dir2_data.c
> > index cb67ec730b9b..bc5c0ba46ec6 100644
> > --- a/fs/xfs/libxfs/xfs_dir2_data.c
> > +++ b/fs/xfs/libxfs/xfs_dir2_data.c
> > @@ -33,6 +33,11 @@
> >  #include "xfs_cksum.h"
> >  #include "xfs_log.h"
> >  
> > +static xfs_failaddr_t xfs_dir2_data_freefind_verify(
> > +		struct xfs_dir2_data_hdr *hdr, struct xfs_dir2_data_free *bf,
> > +		struct xfs_dir2_data_unused *dup,
> > +		struct xfs_dir2_data_free **bf_ent);
> > +
> >  /*
> >   * Check the consistency of the data block.
> >   * The input can also be a block-format directory.
> > @@ -52,6 +57,7 @@ __xfs_dir3_data_check(
> >  	xfs_dir2_data_free_t	*dfp;		/* bestfree entry */
> >  	xfs_dir2_data_unused_t	*dup;		/* unused entry */
> >  	char			*endp;		/* end of useful data */
> > +	xfs_failaddr_t		fa;
> >  	int			freeseen;	/* mask of bestfrees seen */
> >  	xfs_dahash_t		hash;		/* hash of current name */
> >  	int			i;		/* leaf index */
> 
> This could be placed inside the loop scope, right?

Right.

> > -	ASSERT(hdr->magic == cpu_to_be32(XFS_DIR2_DATA_MAGIC) ||
> > -	       hdr->magic == cpu_to_be32(XFS_DIR3_DATA_MAGIC) ||
> > -	       hdr->magic == cpu_to_be32(XFS_DIR2_BLOCK_MAGIC) ||
> > -	       hdr->magic == cpu_to_be32(XFS_DIR3_BLOCK_MAGIC));
> > -	for (dfp = &bf[0], seenzero = matched = 0;
> > -	     dfp < &bf[XFS_DIR2_DATA_FD_COUNT];
> > -	     dfp++) {
> > +	for (dfp = &bf[0]; dfp < &bf[XFS_DIR2_DATA_FD_COUNT]; dfp++) {
> >  		if (!dfp->offset) {
> > -			ASSERT(!dfp->length);
> > -			seenzero = 1;
> > +			if (dfp->length)
> > +				return __this_address;
> > +			seenzero = true;
> >  			continue;
> >  		}
> > -		ASSERT(seenzero == 0);
> > +		if (seenzero)
> > +			return __this_address;
> >  		if (be16_to_cpu(dfp->offset) == off) {
> > -			matched = 1;
> > -			ASSERT(dfp->length == dup->length);
> > -		} else if (off < be16_to_cpu(dfp->offset))
> > -			ASSERT(off + be16_to_cpu(dup->length) <= be16_to_cpu(dfp->offset));
> > -		else
> > -			ASSERT(be16_to_cpu(dfp->offset) + be16_to_cpu(dfp->length) <= off);
> > -		ASSERT(matched || be16_to_cpu(dfp->length) >= be16_to_cpu(dup->length));
> > -		if (dfp > &bf[0])
> > -			ASSERT(be16_to_cpu(dfp[-1].length) >= be16_to_cpu(dfp[0].length));
> > +			matched = true;
> > +			if (dfp->length != dup->length)
> > +				return __this_address;
> > +		} else if (be16_to_cpu(dfp->offset) > off) {
> > +			if (off + be16_to_cpu(dup->length) >
> > +			    be16_to_cpu(dfp->offset))
> 
> can you indent the second line further to indicate it is a
> continuation of the logic statement on the previous line rather than
> a new logic condition? i.e.

Ok.

> 			if (off + be16_to_cpu(dup->length) >
> 					be16_to_cpu(dfp->offset))
> > +				return __this_address;
> > +		} else {
> > +			if (be16_to_cpu(dfp->offset) +
> > +			    be16_to_cpu(dfp->length) > off)
> > +				return __this_address;
> 
> Same here?

Fixed.

> > +		}
> > +		if (!matched &&
> > +		    be16_to_cpu(dfp->length) < be16_to_cpu(dup->length))
> > +			return __this_address;
> > +		if (dfp > &bf[0] &&
> > +		    be16_to_cpu(dfp[-1].length) < be16_to_cpu(dfp[0].length))
> > +			return __this_address;
> >  	}
> > -#endif
> > +
> > +	/*
> > +	 * If this is smaller than the smallest bestfree entry,
> > +	 * it can't be there since they're sorted.
> > +	 */
> > +	if (be16_to_cpu(dup->length) <
> > +	    be16_to_cpu(bf[XFS_DIR2_DATA_FD_COUNT - 1].length))
> > +		return NULL;
> > +	/*
> > +	 * Look at the three bestfree entries for our guy.
> > +	 */
> > +	for (dfp = &bf[0]; dfp < &bf[XFS_DIR2_DATA_FD_COUNT]; dfp++) {
> > +		if (!dfp->offset)
> > +			return NULL;
> > +		if (be16_to_cpu(dfp->offset) == off) {
> > +			*bf_ent = dfp;
> > +			return NULL;
> > +		}
> > +	}
> > +	/*
> > +	 * Didn't find it.  This only happens if there are duplicate lengths.
> > +	 */
> > +	return NULL;
> 
> And this tail is basically a duplicate of what now remains in
> xfs_dir2_data_freefind(). Can you call that function rather than
> duplicating the search code?

Will do.

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 05/10] xfs: don't assert when reporting on-disk corruption while loading btree

[Darrick J. Wong]

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

Don't bother ASSERTing when we're already going to log and return the
corruption status.

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


diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 7b0e2b551e23..368a1179f0ff 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -1248,7 +1248,6 @@ xfs_iread_extents(
 
 		num_recs = xfs_btree_get_numrecs(block);
 		if (unlikely(i + num_recs > nextents)) {
-			ASSERT(i + num_recs <= nextents);
 			xfs_warn(ip->i_mount,
 				"corrupt dinode %Lu, (btree extents).",
 				(unsigned long long) ip->i_ino);

Re: [PATCH 05/10] xfs: don't assert when reporting on-disk corruption while loading btree

[Dave Chinner]

On Sun, Jun 03, 2018 at 04:22:56PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Don't bother ASSERTing when we're already going to log and return the
> corruption status.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Make sense.

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

[PATCH 06/10] xfs: remove redundant ASSERT on insufficient bestfree length in _leaf_addname

[Darrick J. Wong]

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

In xfs_dir2_leaf_addname we ASSERT if the length of the unused space
described by bestfree[0] is less the amount of space we wish to consume.
Immediately after it is a call to xfs_dir2_data_use_free where the
offset parameter is offset of the unused space and the length parameter
is the amount of space we wish to consume.  Both values (and the unused
space pointer) are passed into xfs_dir2_data_check_free, which also
validates that the region of unused space is big enough to cover the
space we wish to consume.  This is effectively the same check that the
ASSERT covers, and since a check failure results in a corruption message
being logged we can remove the ASSERT.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_dir2_leaf.c |    1 -
 1 file changed, 1 deletion(-)


diff --git a/fs/xfs/libxfs/xfs_dir2_leaf.c b/fs/xfs/libxfs/xfs_dir2_leaf.c
index 50fc9c0c5e2b..9367f2a41b35 100644
--- a/fs/xfs/libxfs/xfs_dir2_leaf.c
+++ b/fs/xfs/libxfs/xfs_dir2_leaf.c
@@ -872,7 +872,6 @@ xfs_dir2_leaf_addname(
 	 */
 	dup = (xfs_dir2_data_unused_t *)
 	      ((char *)hdr + be16_to_cpu(bf[0].offset));
-	ASSERT(be16_to_cpu(dup->length) >= length);
 	needscan = needlog = 0;
 	/*
 	 * Mark the initial part of our freespace in use for the new entry.

Re: [PATCH 06/10] xfs: remove redundant ASSERT on insufficient bestfree length in _leaf_addname

[Dave Chinner]

On Sun, Jun 03, 2018 at 04:23:02PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In xfs_dir2_leaf_addname we ASSERT if the length of the unused space
> described by bestfree[0] is less the amount of space we wish to consume.
> Immediately after it is a call to xfs_dir2_data_use_free where the
> offset parameter is offset of the unused space and the length parameter
> is the amount of space we wish to consume.  Both values (and the unused
> space pointer) are passed into xfs_dir2_data_check_free, which also
> validates that the region of unused space is big enough to cover the
> space we wish to consume.  This is effectively the same check that the
> ASSERT covers, and since a check failure results in a corruption message
> being logged we can remove the ASSERT.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks fine.

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

-- 
Dave Chinner
david@fromorbit.com

[PATCH 07/10] xfs: xfs_alloc_get_rec should return EFSCORRUPTED for obvious bnobt corruption

[Darrick J. Wong]

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

Return -EFSCORRUPTED when the bnobt/cntbt return obviously corrupt
values, rather than letting them bounce around in the internal code.

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


diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index dc9dd3805d97..0214a77808d0 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -231,10 +231,14 @@ xfs_alloc_get_rec(
 	int			error;
 
 	error = xfs_btree_get_rec(cur, &rec, stat);
-	if (!error && *stat == 1) {
-		*bno = be32_to_cpu(rec->alloc.ar_startblock);
-		*len = be32_to_cpu(rec->alloc.ar_blockcount);
-	}
+	if (error || !(*stat))
+		return error;
+	if (rec->alloc.ar_blockcount == 0)
+		return -EFSCORRUPTED;
+
+	*bno = be32_to_cpu(rec->alloc.ar_startblock);
+	*len = be32_to_cpu(rec->alloc.ar_blockcount);
+
 	return error;
 }
 

Re: [PATCH 07/10] xfs: xfs_alloc_get_rec should return EFSCORRUPTED for obvious bnobt corruption

[Dave Chinner]

On Sun, Jun 03, 2018 at 04:23:08PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Return -EFSCORRUPTED when the bnobt/cntbt return obviously corrupt
> values, rather than letting them bounce around in the internal code.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_alloc.c |   12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> index dc9dd3805d97..0214a77808d0 100644
> --- a/fs/xfs/libxfs/xfs_alloc.c
> +++ b/fs/xfs/libxfs/xfs_alloc.c
> @@ -231,10 +231,14 @@ xfs_alloc_get_rec(
>  	int			error;
>  
>  	error = xfs_btree_get_rec(cur, &rec, stat);
> -	if (!error && *stat == 1) {
> -		*bno = be32_to_cpu(rec->alloc.ar_startblock);
> -		*len = be32_to_cpu(rec->alloc.ar_blockcount);
> -	}
> +	if (error || !(*stat))
> +		return error;
> +	if (rec->alloc.ar_blockcount == 0)
> +		return -EFSCORRUPTED;
> +
> +	*bno = be32_to_cpu(rec->alloc.ar_startblock);
> +	*len = be32_to_cpu(rec->alloc.ar_blockcount);

Looks good, but makes me wonder if we should verify that
ar_startblock is a valid agbno, and that the extent lies wholly
within the AG? That can be another patch, though.

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

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 07/10] xfs: xfs_alloc_get_rec should return EFSCORRUPTED for obvious bnobt corruption

[Darrick J. Wong]

On Mon, Jun 04, 2018 at 10:14:06AM +1000, Dave Chinner wrote:
> On Sun, Jun 03, 2018 at 04:23:08PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Return -EFSCORRUPTED when the bnobt/cntbt return obviously corrupt
> > values, rather than letting them bounce around in the internal code.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/libxfs/xfs_alloc.c |   12 ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> > index dc9dd3805d97..0214a77808d0 100644
> > --- a/fs/xfs/libxfs/xfs_alloc.c
> > +++ b/fs/xfs/libxfs/xfs_alloc.c
> > @@ -231,10 +231,14 @@ xfs_alloc_get_rec(
> >  	int			error;
> >  
> >  	error = xfs_btree_get_rec(cur, &rec, stat);
> > -	if (!error && *stat == 1) {
> > -		*bno = be32_to_cpu(rec->alloc.ar_startblock);
> > -		*len = be32_to_cpu(rec->alloc.ar_blockcount);
> > -	}
> > +	if (error || !(*stat))
> > +		return error;
> > +	if (rec->alloc.ar_blockcount == 0)
> > +		return -EFSCORRUPTED;
> > +
> > +	*bno = be32_to_cpu(rec->alloc.ar_startblock);
> > +	*len = be32_to_cpu(rec->alloc.ar_blockcount);
> 
> Looks good, but makes me wonder if we should verify that
> ar_startblock is a valid agbno, and that the extent lies wholly
> within the AG? That can be another patch, though.

We probably ought to fix all the _get_rec functions to check that
they're not returning obviously garbage results.

--D

> Reviewed-by: Dave Chinner <dchinner@redhat.com>
> 
> Cheers,
> 
> Dave.
> 
> -- 
> Dave Chinner
> david@fromorbit.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 07/10] xfs: xfs_alloc_get_rec should return EFSCORRUPTED for obvious bnobt corruption

[Dave Chinner]

On Sun, Jun 03, 2018 at 09:32:21PM -0700, Darrick J. Wong wrote:
> On Mon, Jun 04, 2018 at 10:14:06AM +1000, Dave Chinner wrote:
> > On Sun, Jun 03, 2018 at 04:23:08PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > 
> > > Return -EFSCORRUPTED when the bnobt/cntbt return obviously corrupt
> > > values, rather than letting them bounce around in the internal code.
> > > 
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > ---
> > >  fs/xfs/libxfs/xfs_alloc.c |   12 ++++++++----
> > >  1 file changed, 8 insertions(+), 4 deletions(-)
> > > 
> > > 
> > > diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> > > index dc9dd3805d97..0214a77808d0 100644
> > > --- a/fs/xfs/libxfs/xfs_alloc.c
> > > +++ b/fs/xfs/libxfs/xfs_alloc.c
> > > @@ -231,10 +231,14 @@ xfs_alloc_get_rec(
> > >  	int			error;
> > >  
> > >  	error = xfs_btree_get_rec(cur, &rec, stat);
> > > -	if (!error && *stat == 1) {
> > > -		*bno = be32_to_cpu(rec->alloc.ar_startblock);
> > > -		*len = be32_to_cpu(rec->alloc.ar_blockcount);
> > > -	}
> > > +	if (error || !(*stat))
> > > +		return error;
> > > +	if (rec->alloc.ar_blockcount == 0)
> > > +		return -EFSCORRUPTED;
> > > +
> > > +	*bno = be32_to_cpu(rec->alloc.ar_startblock);
> > > +	*len = be32_to_cpu(rec->alloc.ar_blockcount);
> > 
> > Looks good, but makes me wonder if we should verify that
> > ar_startblock is a valid agbno, and that the extent lies wholly
> > within the AG? That can be another patch, though.
> 
> We probably ought to fix all the _get_rec functions to check that
> they're not returning obviously garbage results.

Yup, because that's exactly what the latest fuzzer images are
tripping over - zero'd and/or invalid allocbt records. I'll
make a pass at converting all the _get_rec functions to bounds check
the records they are asked to convert.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

[PATCH 08/10] xfs: btree lookup shouldn't ASSERT on empty btree nodes

[Darrick J. Wong]

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

If a btree lookup encounters an empty btree node or an empty btree leaf
on a multi-level btree, that's evidence of a corrupt on-disk btree.
Therefore, we should return -EFSCORRUPTED to the upper levels, not an
ASSERT failure.

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


diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
index 6101ad685019..4e79baa230f9 100644
--- a/fs/xfs/libxfs/xfs_btree.c
+++ b/fs/xfs/libxfs/xfs_btree.c
@@ -1939,7 +1939,8 @@ xfs_btree_lookup(
 			high = xfs_btree_get_numrecs(block);
 			if (!high) {
 				/* Block is empty, must be an empty leaf. */
-				ASSERT(level == 0 && cur->bc_nlevels == 1);
+				if (level != 0 || cur->bc_nlevels != 1)
+					return -EFSCORRUPTED;
 
 				cur->bc_ptrs[0] = dir != XFS_LOOKUP_LE;
 				*stat = 0;

Re: [PATCH 08/10] xfs: btree lookup shouldn't ASSERT on empty btree nodes

[Dave Chinner]

On Sun, Jun 03, 2018 at 04:23:15PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> If a btree lookup encounters an empty btree node or an empty btree leaf
> on a multi-level btree, that's evidence of a corrupt on-disk btree.
> Therefore, we should return -EFSCORRUPTED to the upper levels, not an
> ASSERT failure.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good.

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

[PATCH 09/10] xfs: don't ASSERT on short form btree root pointer of zero

[Darrick J. Wong]

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

Don't ASSERT if the short form btree root pointer is zero.  Now that we
use xfs_verify_agbno to check all short form btree pointers, we'll let
that log the error and pass it to the upper layers.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_alloc_btree.c    |    1 -
 fs/xfs/libxfs/xfs_refcount_btree.c |    1 -
 fs/xfs/libxfs/xfs_rmap_btree.c     |    1 -
 3 files changed, 3 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_alloc_btree.c b/fs/xfs/libxfs/xfs_alloc_btree.c
index 18aec7a0e599..a4a001f2e130 100644
--- a/fs/xfs/libxfs/xfs_alloc_btree.c
+++ b/fs/xfs/libxfs/xfs_alloc_btree.c
@@ -242,7 +242,6 @@ xfs_allocbt_init_ptr_from_cur(
 	struct xfs_agf		*agf = XFS_BUF_TO_AGF(cur->bc_private.a.agbp);
 
 	ASSERT(cur->bc_private.a.agno == be32_to_cpu(agf->agf_seqno));
-	ASSERT(agf->agf_roots[cur->bc_btnum] != 0);
 
 	ptr->s = agf->agf_roots[cur->bc_btnum];
 }
diff --git a/fs/xfs/libxfs/xfs_refcount_btree.c b/fs/xfs/libxfs/xfs_refcount_btree.c
index 375abfeb6267..f652b48128c3 100644
--- a/fs/xfs/libxfs/xfs_refcount_btree.c
+++ b/fs/xfs/libxfs/xfs_refcount_btree.c
@@ -192,7 +192,6 @@ xfs_refcountbt_init_ptr_from_cur(
 	struct xfs_agf		*agf = XFS_BUF_TO_AGF(cur->bc_private.a.agbp);
 
 	ASSERT(cur->bc_private.a.agno == be32_to_cpu(agf->agf_seqno));
-	ASSERT(agf->agf_refcount_root != 0);
 
 	ptr->s = agf->agf_refcount_root;
 }
diff --git a/fs/xfs/libxfs/xfs_rmap_btree.c b/fs/xfs/libxfs/xfs_rmap_btree.c
index d756e0b84abf..78ab00beee93 100644
--- a/fs/xfs/libxfs/xfs_rmap_btree.c
+++ b/fs/xfs/libxfs/xfs_rmap_btree.c
@@ -234,7 +234,6 @@ xfs_rmapbt_init_ptr_from_cur(
 	struct xfs_agf		*agf = XFS_BUF_TO_AGF(cur->bc_private.a.agbp);
 
 	ASSERT(cur->bc_private.a.agno == be32_to_cpu(agf->agf_seqno));
-	ASSERT(agf->agf_roots[cur->bc_btnum] != 0);
 
 	ptr->s = agf->agf_roots[cur->bc_btnum];
 }

Re: [PATCH 09/10] xfs: don't ASSERT on short form btree root pointer of zero

[Dave Chinner]

On Sun, Jun 03, 2018 at 04:23:21PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Don't ASSERT if the short form btree root pointer is zero.  Now that we
> use xfs_verify_agbno to check all short form btree pointers, we'll let
> that log the error and pass it to the upper layers.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Makes sense.

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

[PATCH 10/10] xfs: don't return garbage buffers in xfs_da3_node_read

[Darrick J. Wong]

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

If we're reading a node in a dir/attr btree and the buffer comes off the
disk with a magic number we don't recognize, don't ASSERT and don't set
a garbage buffer type (0 also triggers ASSERTs).  Instead, report the
corruption, release the buffer, and return -EFSCORRUPTED because that's
what the dabtree is -- corrupt.

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


diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
index ea187b4a7991..39c1013358ed 100644
--- a/fs/xfs/libxfs/xfs_da_btree.c
+++ b/fs/xfs/libxfs/xfs_da_btree.c
@@ -305,9 +305,11 @@ xfs_da3_node_read(
 			type = XFS_BLFT_DIR_LEAFN_BUF;
 			break;
 		default:
-			type = 0;
-			ASSERT(0);
-			break;
+			XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
+					tp->t_mountp, info);
+			xfs_trans_brelse(tp, *bpp);
+			*bpp = NULL;
+			return -EFSCORRUPTED;
 		}
 		xfs_trans_buf_set_type(tp, *bpp, type);
 	}

Re: [PATCH 10/10] xfs: don't return garbage buffers in xfs_da3_node_read

[Dave Chinner]

On Sun, Jun 03, 2018 at 04:23:27PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> If we're reading a node in a dir/attr btree and the buffer comes off the
> disk with a magic number we don't recognize, don't ASSERT and don't set
> a garbage buffer type (0 also triggers ASSERTs).  Instead, report the
> corruption, release the buffer, and return -EFSCORRUPTED because that's
> what the dabtree is -- corrupt.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Makes sense. I'm kinda surprised we haven't tripped over this given
that it looks like an obvious fuzzer target....

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