[PATCH 0/5] xfs: various 4.15 scrub fixes

[PATCH 0/5] xfs: various 4.15 scrub fixes

[Darrick J. Wong]

Hi all,

Here are a few more online scrub patches that I would like to include
for 4.15.  The first patch fixes a problem where we failed to detect
duplicate block pointers in the AGFL; it's the same patch I sent solo
last week.  The four that come after it fix most of the smatch and
sparse complaints that came up over the weekend.

(smatch still complains about a check-after-use error in dabtree.c, but
I've yet to figure that one out...)

--Darrick

[PATCH 1/5] xfs: check the uniqueness of the AGFL entries

[Darrick J. Wong]

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

Make sure we don't list a block twice in the agfl.

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


diff --git a/fs/xfs/scrub/agheader.c b/fs/xfs/scrub/agheader.c
index 5495aa5..a7d514b 100644
--- a/fs/xfs/scrub/agheader.c
+++ b/fs/xfs/scrub/agheader.c
@@ -476,6 +476,11 @@ xfs_scrub_agf(
 
 /* AGFL */
 
+struct xfs_scrub_agfl_info {
+	unsigned int			nr_entries;
+	xfs_agblock_t			*entries;
+};
+
 /* Scrub an AGFL block. */
 STATIC int
 xfs_scrub_agfl_block(
@@ -484,20 +489,39 @@ xfs_scrub_agfl_block(
 	void				*priv)
 {
 	struct xfs_mount		*mp = sc->mp;
+	struct xfs_scrub_agfl_info	*sai = priv;
 	xfs_agnumber_t			agno = sc->sa.agno;
 
-	if (!xfs_verify_agbno(mp, agno, agbno))
+	if (xfs_verify_agbno(mp, agno, agbno))
+		sai->entries[sai->nr_entries++] = agbno;
+	else
 		xfs_scrub_block_set_corrupt(sc, sc->sa.agfl_bp);
 
 	return 0;
 }
 
+static int
+xfs_scrub_agblock_cmp(
+	const void		*pa,
+	const void		*pb)
+{
+	const xfs_agblock_t	*a = pa;
+	const xfs_agblock_t	*b = pb;
+
+	return (int)*a - (int)*b;
+}
+
 /* Scrub the AGFL. */
 int
 xfs_scrub_agfl(
 	struct xfs_scrub_context	*sc)
 {
+	struct xfs_scrub_agfl_info	sai = { 0 };
+	struct xfs_agf			*agf;
+	size_t				sz;
 	xfs_agnumber_t			agno;
+	unsigned int			agflcount;
+	unsigned int			i;
 	int				error;
 
 	agno = sc->sa.agno = sc->sm->sm_agno;
@@ -508,8 +532,33 @@ xfs_scrub_agfl(
 	if (!sc->sa.agf_bp)
 		return -EFSCORRUPTED;
 
+	/* Allocate buffer to ensure uniqueness of AGFL entries. */
+	agf = XFS_BUF_TO_AGF(sc->sa.agf_bp);
+	agflcount = be32_to_cpu(agf->agf_flcount);
+	sz = sizeof(xfs_agblock_t) * agflcount;
+	if (sz > sc->mp->m_sb.sb_sectsize) {
+		xfs_scrub_block_set_corrupt(sc, sc->sa.agf_bp);
+		goto out;
+	}
+	sai.entries = kmem_zalloc(sz, KM_NOFS);
+
 	/* Check the blocks in the AGFL. */
-	return xfs_scrub_walk_agfl(sc, xfs_scrub_agfl_block, NULL);
+	error = xfs_scrub_walk_agfl(sc, xfs_scrub_agfl_block, &sai);
+	if (error)
+		goto out_free;
+
+	/* Sort entries, check for duplicates. */
+	sort(sai.entries, sai.nr_entries, sizeof(sai.entries[0]),
+			xfs_scrub_agblock_cmp, NULL);
+	for (i = 1; i < sai.nr_entries; i++) {
+		if (sai.entries[i] == sai.entries[i - 1]) {
+			xfs_scrub_block_set_corrupt(sc, sc->sa.agf_bp);
+			break;
+		}
+	}
+
+out_free:
+	kmem_free(sai.entries);
 out:
 	return error;
 }

[PATCH 2/5] xfs: refactor the directory data block bestfree checks

[Darrick J. Wong]

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

In a directory data block, the zeroth bestfree item must point to the
longest free space.  Therefore, when we check the bestfree block's
records against the data blocks, we only need to compare with bf[0] and
don't need the loop.

The weird loop was most probably the result of an earlier refactoring
gone bad.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/scrub/dir.c |   20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)


diff --git a/fs/xfs/scrub/dir.c b/fs/xfs/scrub/dir.c
index d4cd766..c8ca3fd 100644
--- a/fs/xfs/scrub/dir.c
+++ b/fs/xfs/scrub/dir.c
@@ -435,25 +435,15 @@ xfs_scrub_directory_check_freesp(
 	struct xfs_buf			*dbp,
 	unsigned int			len)
 {
-	struct xfs_dir2_data_free	*bf;
 	struct xfs_dir2_data_free	*dfp;
-	int				offset;
 
-	if (len == 0)
-		return;
+	dfp = sc->ip->d_ops->data_bestfree_p(dbp->b_addr);
 
-	bf = sc->ip->d_ops->data_bestfree_p(dbp->b_addr);
-	for (dfp = &bf[0]; dfp < &bf[XFS_DIR2_DATA_FD_COUNT]; dfp++) {
-		offset = be16_to_cpu(dfp->offset);
-		if (offset == 0)
-			break;
-		if (len == be16_to_cpu(dfp->length))
-			return;
-		/* Didn't find the best length in the bestfree data */
-		break;
-	}
+	if (len != be16_to_cpu(dfp->length))
+		xfs_scrub_fblock_set_corrupt(sc, XFS_DATA_FORK, lblk);
 
-	xfs_scrub_fblock_set_corrupt(sc, XFS_DATA_FORK, lblk);
+	if (len > 0 && be16_to_cpu(dfp->offset) == 0)
+		xfs_scrub_fblock_set_corrupt(sc, XFS_DATA_FORK, lblk);
 }
 
 /* Check free space info in a directory leaf1 block. */

[PATCH 3/5] xfs: pass inode number to xfs_scrub_ino_set_{preen, warning}

[Darrick J. Wong]

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

There are two ways to scrub an inode -- calling xfs_iget and checking
the raw inode core, or by loading the inode cluster buffer and checking
the on-disk contents directly.  The second method is only useful if
_iget fails the verifiers; when this is the case, sc->ip is NULL and
calling the tracepoint will cause a system crash.

Therefore, pass the raw inode number directly into the _preen and
_warning functions.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/scrub/attr.c   |    2 +-
 fs/xfs/scrub/common.c |    6 ++++--
 fs/xfs/scrub/common.h |    5 +++--
 fs/xfs/scrub/inode.c  |    8 ++++----
 4 files changed, 12 insertions(+), 9 deletions(-)


diff --git a/fs/xfs/scrub/attr.c b/fs/xfs/scrub/attr.c
index 5cf30de..4ed8047 100644
--- a/fs/xfs/scrub/attr.c
+++ b/fs/xfs/scrub/attr.c
@@ -98,7 +98,7 @@ xfs_scrub_xattr_listent(
 
 	if (flags & XFS_ATTR_INCOMPLETE) {
 		/* Incomplete attr key, just mark the inode for preening. */
-		xfs_scrub_ino_set_preen(sx->sc, NULL);
+		xfs_scrub_ino_set_preen(sx->sc, context->dp->i_ino, NULL);
 		return;
 	}
 
diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c
index 318dd97..ac95fe9 100644
--- a/fs/xfs/scrub/common.c
+++ b/fs/xfs/scrub/common.c
@@ -165,10 +165,11 @@ xfs_scrub_block_set_preen(
 void
 xfs_scrub_ino_set_preen(
 	struct xfs_scrub_context	*sc,
+	xfs_ino_t			ino,
 	struct xfs_buf			*bp)
 {
 	sc->sm->sm_flags |= XFS_SCRUB_OFLAG_PREEN;
-	trace_xfs_scrub_ino_preen(sc, sc->ip->i_ino, bp ? bp->b_bn : 0,
+	trace_xfs_scrub_ino_preen(sc, ino, bp ? bp->b_bn : 0,
 			__return_address);
 }
 
@@ -215,10 +216,11 @@ xfs_scrub_fblock_set_corrupt(
 void
 xfs_scrub_ino_set_warning(
 	struct xfs_scrub_context	*sc,
+	xfs_ino_t			ino,
 	struct xfs_buf			*bp)
 {
 	sc->sm->sm_flags |= XFS_SCRUB_OFLAG_WARNING;
-	trace_xfs_scrub_ino_warning(sc, sc->ip->i_ino, bp ? bp->b_bn : 0,
+	trace_xfs_scrub_ino_warning(sc, ino, bp ? bp->b_bn : 0,
 			__return_address);
 }
 
diff --git a/fs/xfs/scrub/common.h b/fs/xfs/scrub/common.h
index 0409ec2..5c04385 100644
--- a/fs/xfs/scrub/common.h
+++ b/fs/xfs/scrub/common.h
@@ -58,7 +58,8 @@ bool xfs_scrub_fblock_process_error(struct xfs_scrub_context *sc, int whichfork,
 
 void xfs_scrub_block_set_preen(struct xfs_scrub_context *sc,
 		struct xfs_buf *bp);
-void xfs_scrub_ino_set_preen(struct xfs_scrub_context *sc, struct xfs_buf *bp);
+void xfs_scrub_ino_set_preen(struct xfs_scrub_context *sc, xfs_ino_t ino,
+		struct xfs_buf *bp);
 
 void xfs_scrub_block_set_corrupt(struct xfs_scrub_context *sc,
 		struct xfs_buf *bp);
@@ -67,7 +68,7 @@ void xfs_scrub_ino_set_corrupt(struct xfs_scrub_context *sc, xfs_ino_t ino,
 void xfs_scrub_fblock_set_corrupt(struct xfs_scrub_context *sc, int whichfork,
 		xfs_fileoff_t offset);
 
-void xfs_scrub_ino_set_warning(struct xfs_scrub_context *sc,
+void xfs_scrub_ino_set_warning(struct xfs_scrub_context *sc, xfs_ino_t ino,
 		struct xfs_buf *bp);
 void xfs_scrub_fblock_set_warning(struct xfs_scrub_context *sc, int whichfork,
 		xfs_fileoff_t offset);
diff --git a/fs/xfs/scrub/inode.c b/fs/xfs/scrub/inode.c
index f275dd2..637b7a8 100644
--- a/fs/xfs/scrub/inode.c
+++ b/fs/xfs/scrub/inode.c
@@ -328,7 +328,7 @@ xfs_scrub_dinode(
 		 * We autoconvert v1 inodes into v2 inodes on writeout,
 		 * so just mark this inode for preening.
 		 */
-		xfs_scrub_ino_set_preen(sc, bp);
+		xfs_scrub_ino_set_preen(sc, ino, bp);
 		break;
 	case 2:
 	case 3:
@@ -353,7 +353,7 @@ xfs_scrub_dinode(
 	 */
 	if (dip->di_uid == cpu_to_be32(-1U) ||
 	    dip->di_gid == cpu_to_be32(-1U))
-		xfs_scrub_ino_set_warning(sc, bp);
+		xfs_scrub_ino_set_warning(sc, ino, bp);
 
 	/* di_format */
 	switch (dip->di_format) {
@@ -408,7 +408,7 @@ xfs_scrub_dinode(
 	 * overly large offsets, flag the inode for admin review.
 	 */
 	if (isize >= mp->m_super->s_maxbytes)
-		xfs_scrub_ino_set_warning(sc, bp);
+		xfs_scrub_ino_set_warning(sc, ino, bp);
 
 	/* di_nblocks */
 	if (flags2 & XFS_DIFLAG2_REFLINK) {
@@ -601,7 +601,7 @@ xfs_scrub_inode(
 				XFS_INO_TO_AGBNO(mp, ino), &error))
 			goto out;
 		if (!has_shared)
-			xfs_scrub_ino_set_preen(sc, bp);
+			xfs_scrub_ino_set_preen(sc, ino, bp);
 	}
 
 out:

[PATCH 4/5] xfs: fix uninitialized return values in scrub code

[Darrick J. Wong]

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

Fix smatch complaints about uninitialized return codes.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/scrub/dir.c    |    2 +-
 fs/xfs/scrub/parent.c |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)


diff --git a/fs/xfs/scrub/dir.c b/fs/xfs/scrub/dir.c
index c8ca3fd..69e1efd 100644
--- a/fs/xfs/scrub/dir.c
+++ b/fs/xfs/scrub/dir.c
@@ -748,7 +748,7 @@ xfs_scrub_directory(
 	};
 	size_t				bufsize;
 	loff_t				oldpos;
-	int				error;
+	int				error = 0;
 
 	if (!S_ISDIR(VFS_I(sc->ip)->i_mode))
 		return -ENOENT;
diff --git a/fs/xfs/scrub/parent.c b/fs/xfs/scrub/parent.c
index cc2b8f6..63a2533 100644
--- a/fs/xfs/scrub/parent.c
+++ b/fs/xfs/scrub/parent.c
@@ -143,7 +143,7 @@ xfs_scrub_parent_validate(
 	struct xfs_inode		*dp = NULL;
 	xfs_nlink_t			expected_nlink;
 	xfs_nlink_t			nlink;
-	int				error;
+	int				error = 0;
 
 	*try_again = false;
 
@@ -258,7 +258,7 @@ xfs_scrub_parent(
 	xfs_ino_t			dnum;
 	bool				try_again;
 	int				tries = 0;
-	int				error;
+	int				error = 0;
 
 	/*
 	 * If we're a directory, check that the '..' link points up to

[PATCH 5/5] xfs: fix btree scrub deref check

[Darrick J. Wong]

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

The btree scrubber has some custom code to retrieve and check a btree
block via xfs_btree_lookup_get_block.  This function will either return
an error code (verifiers failed) or a *pblock will be untouched (bad
pointer).  Since we previously set *pblock to NULL, we need to check
*pblock, not pblock, to trigger the early bailout.

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


diff --git a/fs/xfs/scrub/btree.c b/fs/xfs/scrub/btree.c
index a814404..df07661 100644
--- a/fs/xfs/scrub/btree.c
+++ b/fs/xfs/scrub/btree.c
@@ -335,7 +335,7 @@ xfs_scrub_btree_get_block(
 
 	error = xfs_btree_lookup_get_block(bs->cur, level, pp, pblock);
 	if (!xfs_scrub_btree_process_error(bs->sc, bs->cur, level, &error) ||
-	    !pblock)
+	    !*pblock)
 		return error;
 
 	xfs_btree_get_block(bs->cur, level, pbp);

[PATCH 6/5] xfs: only check da node header padding on v5 filesystems

[Darrick J. Wong]

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

It turns out that we only started zeroing a new da btree node's block
header on v5 filesystems.  Prior to that, we just wouldn't set anything
at all, which means that the pad field never got set and would retain
whatever happened to be in memory.

Therefore, we can only check the pad for zeroness on v5 filesystems.
shared/006 on a v4 filesystem exposes this scrub bug.

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

diff --git a/fs/xfs/scrub/dabtree.c b/fs/xfs/scrub/dabtree.c
index 4c9839c..d94edd9 100644
--- a/fs/xfs/scrub/dabtree.c
+++ b/fs/xfs/scrub/dabtree.c
@@ -378,7 +378,8 @@ xfs_scrub_da_btree_block(
 	blk->magic = be16_to_cpu(hdr3->hdr.magic);
 	pmaxrecs = &ds->maxrecs[level];
 
-	if (hdr3->hdr.pad != cpu_to_be16(0))
+	/* We only started zeroing the header on v5 filesystems. */
+	if (xfs_sb_version_hascrc(&ds->sc->mp->m_sb) && hdr3->hdr.pad)
 		xfs_scrub_da_set_corrupt(ds, level);
 
 	/* Check the owner. */

Re: [PATCH 1/5] xfs: check the uniqueness of the AGFL entries

[Dave Chinner]

On Mon, Nov 06, 2017 at 05:03:27PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Make sure we don't list a block twice in the agfl.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
.....
>  /* Scrub the AGFL. */
>  int
>  xfs_scrub_agfl(
>  	struct xfs_scrub_context	*sc)
>  {
> +	struct xfs_scrub_agfl_info	sai = { 0 };
> +	struct xfs_agf			*agf;
> +	size_t				sz;
>  	xfs_agnumber_t			agno;
> +	unsigned int			agflcount;
> +	unsigned int			i;
>  	int				error;
>  
>  	agno = sc->sa.agno = sc->sm->sm_agno;
> @@ -508,8 +532,33 @@ xfs_scrub_agfl(
>  	if (!sc->sa.agf_bp)
>  		return -EFSCORRUPTED;
>  
> +	/* Allocate buffer to ensure uniqueness of AGFL entries. */
> +	agf = XFS_BUF_TO_AGF(sc->sa.agf_bp);
> +	agflcount = be32_to_cpu(agf->agf_flcount);
> +	sz = sizeof(xfs_agblock_t) * agflcount;
> +	if (sz > sc->mp->m_sb.sb_sectsize) {

No need for sz?

	if (agflcount > XFS_AGFL_SIZE(mp)

> +		xfs_scrub_block_set_corrupt(sc, sc->sa.agf_bp);
> +		goto out;
> +	}
> +	sai.entries = kmem_zalloc(sz, KM_NOFS);

KM_NOSLEEP, check for failure?

Otherwise looks ok.

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/5] xfs: refactor the directory data block bestfree checks

[Dave Chinner]

On Mon, Nov 06, 2017 at 05:03:33PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In a directory data block, the zeroth bestfree item must point to the
> longest free space.  Therefore, when we check the bestfree block's
> records against the data blocks, we only need to compare with bf[0] and
> don't need the loop.
> 
> The weird loop was most probably the result of an earlier refactoring
> gone bad.
> 
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good.


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

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 3/5] xfs: pass inode number to xfs_scrub_ino_set_{preen, warning}

[Dave Chinner]

On Mon, Nov 06, 2017 at 05:03:40PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> There are two ways to scrub an inode -- calling xfs_iget and checking
> the raw inode core, or by loading the inode cluster buffer and checking
> the on-disk contents directly.  The second method is only useful if
> _iget fails the verifiers; when this is the case, sc->ip is NULL and
> calling the tracepoint will cause a system crash.
> 
> Therefore, pass the raw inode number directly into the _preen and
> _warning functions.
> 
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Simple enough. Looks ok

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

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 4/5] xfs: fix uninitialized return values in scrub code

[Dave Chinner]

On Mon, Nov 06, 2017 at 05:03:46PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Fix smatch complaints about uninitialized return codes.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

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

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 5/5] xfs: fix btree scrub deref check

[Dave Chinner]

On Mon, Nov 06, 2017 at 05:03:52PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> The btree scrubber has some custom code to retrieve and check a btree
> block via xfs_btree_lookup_get_block.  This function will either return
> an error code (verifiers failed) or a *pblock will be untouched (bad
> pointer).  Since we previously set *pblock to NULL, we need to check
> *pblock, not pblock, to trigger the early bailout.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Ah, the dangers of double pointers. Looks good.

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

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 6/5] xfs: only check da node header padding on v5 filesystems

[Dave Chinner]

On Wed, Nov 08, 2017 at 12:56:36PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> It turns out that we only started zeroing a new da btree node's block
> header on v5 filesystems.  Prior to that, we just wouldn't set anything
> at all, which means that the pad field never got set and would retain
> whatever happened to be in memory.
> 
> Therefore, we can only check the pad for zeroness on v5 filesystems.
> shared/006 on a v4 filesystem exposes this scrub bug.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

looks good.


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

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/5] xfs: check the uniqueness of the AGFL entries

[Darrick J. Wong]

On Thu, Nov 09, 2017 at 01:08:29PM +1100, Dave Chinner wrote:
> On Mon, Nov 06, 2017 at 05:03:27PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Make sure we don't list a block twice in the agfl.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> .....
> >  /* Scrub the AGFL. */
> >  int
> >  xfs_scrub_agfl(
> >  	struct xfs_scrub_context	*sc)
> >  {
> > +	struct xfs_scrub_agfl_info	sai = { 0 };
> > +	struct xfs_agf			*agf;
> > +	size_t				sz;
> >  	xfs_agnumber_t			agno;
> > +	unsigned int			agflcount;
> > +	unsigned int			i;
> >  	int				error;
> >  
> >  	agno = sc->sa.agno = sc->sm->sm_agno;
> > @@ -508,8 +532,33 @@ xfs_scrub_agfl(
> >  	if (!sc->sa.agf_bp)
> >  		return -EFSCORRUPTED;
> >  
> > +	/* Allocate buffer to ensure uniqueness of AGFL entries. */
> > +	agf = XFS_BUF_TO_AGF(sc->sa.agf_bp);
> > +	agflcount = be32_to_cpu(agf->agf_flcount);
> > +	sz = sizeof(xfs_agblock_t) * agflcount;
> > +	if (sz > sc->mp->m_sb.sb_sectsize) {
> 
> No need for sz?
> 
> 	if (agflcount > XFS_AGFL_SIZE(mp)

<nod>

> > +		xfs_scrub_block_set_corrupt(sc, sc->sa.agf_bp);
> > +		goto out;
> > +	}
> > +	sai.entries = kmem_zalloc(sz, KM_NOFS);
> 
> KM_NOSLEEP, check for failure?

Ok.

--D

> Otherwise looks ok.
> 
> -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 v2 1/5] xfs: check the uniqueness of the AGFL entries

[Darrick J. Wong]

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

Make sure we don't list a block twice in the agfl.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
v2: various review cleanups
---
 fs/xfs/scrub/agheader.c |   56 +++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 54 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/scrub/agheader.c b/fs/xfs/scrub/agheader.c
index 5495aa5..489f985 100644
--- a/fs/xfs/scrub/agheader.c
+++ b/fs/xfs/scrub/agheader.c
@@ -476,6 +476,11 @@ xfs_scrub_agf(
 
 /* AGFL */
 
+struct xfs_scrub_agfl_info {
+	unsigned int			nr_entries;
+	xfs_agblock_t			*entries;
+};
+
 /* Scrub an AGFL block. */
 STATIC int
 xfs_scrub_agfl_block(
@@ -484,20 +489,38 @@ xfs_scrub_agfl_block(
 	void				*priv)
 {
 	struct xfs_mount		*mp = sc->mp;
+	struct xfs_scrub_agfl_info	*sai = priv;
 	xfs_agnumber_t			agno = sc->sa.agno;
 
-	if (!xfs_verify_agbno(mp, agno, agbno))
+	if (xfs_verify_agbno(mp, agno, agbno))
+		sai->entries[sai->nr_entries++] = agbno;
+	else
 		xfs_scrub_block_set_corrupt(sc, sc->sa.agfl_bp);
 
 	return 0;
 }
 
+static int
+xfs_scrub_agblock_cmp(
+	const void		*pa,
+	const void		*pb)
+{
+	const xfs_agblock_t	*a = pa;
+	const xfs_agblock_t	*b = pb;
+
+	return (int)*a - (int)*b;
+}
+
 /* Scrub the AGFL. */
 int
 xfs_scrub_agfl(
 	struct xfs_scrub_context	*sc)
 {
+	struct xfs_scrub_agfl_info	sai = { 0 };
+	struct xfs_agf			*agf;
 	xfs_agnumber_t			agno;
+	unsigned int			agflcount;
+	unsigned int			i;
 	int				error;
 
 	agno = sc->sa.agno = sc->sm->sm_agno;
@@ -508,8 +531,37 @@ xfs_scrub_agfl(
 	if (!sc->sa.agf_bp)
 		return -EFSCORRUPTED;
 
+	/* Allocate buffer to ensure uniqueness of AGFL entries. */
+	agf = XFS_BUF_TO_AGF(sc->sa.agf_bp);
+	agflcount = be32_to_cpu(agf->agf_flcount);
+	if (agflcount > XFS_AGFL_SIZE(sc->mp)) {
+		xfs_scrub_block_set_corrupt(sc, sc->sa.agf_bp);
+		goto out;
+	}
+	sai.entries = kmem_zalloc(sizeof(xfs_agblock_t) * agflcount,
+			KM_SLEEP | KM_NOFS);
+	if (!sai.entries) {
+		error = -ENOMEM;
+		goto out;
+	}
+
 	/* Check the blocks in the AGFL. */
-	return xfs_scrub_walk_agfl(sc, xfs_scrub_agfl_block, NULL);
+	error = xfs_scrub_walk_agfl(sc, xfs_scrub_agfl_block, &sai);
+	if (error)
+		goto out_free;
+
+	/* Sort entries, check for duplicates. */
+	sort(sai.entries, sai.nr_entries, sizeof(sai.entries[0]),
+			xfs_scrub_agblock_cmp, NULL);
+	for (i = 1; i < sai.nr_entries; i++) {
+		if (sai.entries[i] == sai.entries[i - 1]) {
+			xfs_scrub_block_set_corrupt(sc, sc->sa.agf_bp);
+			break;
+		}
+	}
+
+out_free:
+	kmem_free(sai.entries);
 out:
 	return error;
 }

[PATCH 7/5] xfs: on failed mount, force-reclaim inodes after unmounting quota controls

[Darrick J. Wong]

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

When mounting fails, we must force-reclaim inodes (and disable delayed
reclaim) /after/ the realtime and quota control have let go of the
realtime and quota inodes.  Without this, we corrupt the timer list and
cause other weird problems.

Found by xfs/376 fuzzing u3.bmbt[0].lastoff on an rmap filesystem to
force a bogus post-eof extent reclaim that causes the fs to go down.

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

diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index e9727d0..a9f0fe9 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -1022,10 +1022,15 @@ xfs_mountfs(
 	xfs_rtunmount_inodes(mp);
  out_rele_rip:
 	IRELE(rip);
-	cancel_delayed_work_sync(&mp->m_reclaim_work);
-	xfs_reclaim_inodes(mp, SYNC_WAIT);
 	/* Clean out dquots that might be in memory after quotacheck. */
 	xfs_qm_unmount(mp);
+	/*
+	 * Cancel all delayed reclaim work and reclaim the inodes directly.
+	 * We have to do this /after/ rtunmount and qm_unmount because those
+	 * two will have scheduled delayed reclaim for the rt/quota inodes.
+	 */
+	cancel_delayed_work_sync(&mp->m_reclaim_work);
+	xfs_reclaim_inodes(mp, SYNC_WAIT);
  out_log_dealloc:
 	mp->m_flags |= XFS_MOUNT_UNMOUNTING;
 	xfs_log_mount_cancel(mp);

[PATCH 8/5] xfs: remove u_int* type usage

[Darrick J. Wong]

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

Use the uint* types instead of the u_int* types.  This will (hopefully)
pair with an xfsprogs cleanup.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_format.h |    2 +-
 fs/xfs/xfs_attr_list.c     |    4 ++--
 fs/xfs/xfs_ioctl.c         |    4 ++--
 fs/xfs/xfs_ioctl.h         |    4 ++--
 4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
index 2e047e7..1acb584 100644
--- a/fs/xfs/libxfs/xfs_format.h
+++ b/fs/xfs/libxfs/xfs_format.h
@@ -1147,7 +1147,7 @@ static inline void xfs_dinode_put_rdev(struct xfs_dinode *dip, xfs_dev_t rdev)
  * Dquot and dquot block format definitions
  */
 #define XFS_DQUOT_MAGIC		0x4451		/* 'DQ' */
-#define XFS_DQUOT_VERSION	(u_int8_t)0x01	/* latest version number */
+#define XFS_DQUOT_VERSION	(uint8_t)0x01	/* latest version number */
 
 /*
  * This is the main portion of the on-disk representation of quota
diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c
index a360310..3e59a34 100644
--- a/fs/xfs/xfs_attr_list.c
+++ b/fs/xfs/xfs_attr_list.c
@@ -546,8 +546,8 @@ xfs_attr_list_int(
 #define	ATTR_ENTBASESIZE		/* minimum bytes used by an attr */ \
 	(((struct attrlist_ent *) 0)->a_name - (char *) 0)
 #define	ATTR_ENTSIZE(namelen)		/* actual bytes used by an attr */ \
-	((ATTR_ENTBASESIZE + (namelen) + 1 + sizeof(u_int32_t)-1) \
-	 & ~(sizeof(u_int32_t)-1))
+	((ATTR_ENTBASESIZE + (namelen) + 1 + sizeof(uint32_t)-1) \
+	 & ~(sizeof(uint32_t)-1))
 
 /*
  * Format an attribute and copy it out to the user's buffer.
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 65a7951..20dc65f 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -311,8 +311,8 @@ xfs_readlink_by_handle(
 int
 xfs_set_dmattrs(
 	xfs_inode_t     *ip,
-	u_int		evmask,
-	u_int16_t	state)
+	uint		evmask,
+	uint16_t	state)
 {
 	xfs_mount_t	*mp = ip->i_mount;
 	xfs_trans_t	*tp;
diff --git a/fs/xfs/xfs_ioctl.h b/fs/xfs/xfs_ioctl.h
index e86c3ea..8de879f 100644
--- a/fs/xfs/xfs_ioctl.h
+++ b/fs/xfs/xfs_ioctl.h
@@ -86,7 +86,7 @@ xfs_file_compat_ioctl(
 extern int
 xfs_set_dmattrs(
 	struct xfs_inode	*ip,
-	u_int			evmask,
-	u_int16_t		state);
+	uint			evmask,
+	uint16_t		state);
 
 #endif

Re: [PATCH v2 1/5] xfs: check the uniqueness of the AGFL entries

[Darrick J. Wong]

On Wed, Nov 08, 2017 at 09:02:35PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Make sure we don't list a block twice in the agfl.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> v2: various review cleanups
> ---
>  fs/xfs/scrub/agheader.c |   56 +++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 54 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/scrub/agheader.c b/fs/xfs/scrub/agheader.c
> index 5495aa5..489f985 100644
> --- a/fs/xfs/scrub/agheader.c
> +++ b/fs/xfs/scrub/agheader.c
> @@ -476,6 +476,11 @@ xfs_scrub_agf(
>  
>  /* AGFL */
>  
> +struct xfs_scrub_agfl_info {
> +	unsigned int			nr_entries;
> +	xfs_agblock_t			*entries;
> +};
> +
>  /* Scrub an AGFL block. */
>  STATIC int
>  xfs_scrub_agfl_block(
> @@ -484,20 +489,38 @@ xfs_scrub_agfl_block(
>  	void				*priv)
>  {
>  	struct xfs_mount		*mp = sc->mp;
> +	struct xfs_scrub_agfl_info	*sai = priv;
>  	xfs_agnumber_t			agno = sc->sa.agno;
>  
> -	if (!xfs_verify_agbno(mp, agno, agbno))
> +	if (xfs_verify_agbno(mp, agno, agbno))
> +		sai->entries[sai->nr_entries++] = agbno;

self-NAK on this second version, because we need to ensure that we don't
overflow or underflow the entries array -- in other words, that flfirst
and fllast exactly match flcount.

--D

> +	else
>  		xfs_scrub_block_set_corrupt(sc, sc->sa.agfl_bp);
>  
>  	return 0;
>  }
>  
> +static int
> +xfs_scrub_agblock_cmp(
> +	const void		*pa,
> +	const void		*pb)
> +{
> +	const xfs_agblock_t	*a = pa;
> +	const xfs_agblock_t	*b = pb;
> +
> +	return (int)*a - (int)*b;
> +}
> +
>  /* Scrub the AGFL. */
>  int
>  xfs_scrub_agfl(
>  	struct xfs_scrub_context	*sc)
>  {
> +	struct xfs_scrub_agfl_info	sai = { 0 };
> +	struct xfs_agf			*agf;
>  	xfs_agnumber_t			agno;
> +	unsigned int			agflcount;
> +	unsigned int			i;
>  	int				error;
>  
>  	agno = sc->sa.agno = sc->sm->sm_agno;
> @@ -508,8 +531,37 @@ xfs_scrub_agfl(
>  	if (!sc->sa.agf_bp)
>  		return -EFSCORRUPTED;
>  
> +	/* Allocate buffer to ensure uniqueness of AGFL entries. */
> +	agf = XFS_BUF_TO_AGF(sc->sa.agf_bp);
> +	agflcount = be32_to_cpu(agf->agf_flcount);
> +	if (agflcount > XFS_AGFL_SIZE(sc->mp)) {
> +		xfs_scrub_block_set_corrupt(sc, sc->sa.agf_bp);
> +		goto out;
> +	}
> +	sai.entries = kmem_zalloc(sizeof(xfs_agblock_t) * agflcount,
> +			KM_SLEEP | KM_NOFS);
> +	if (!sai.entries) {
> +		error = -ENOMEM;
> +		goto out;
> +	}
> +
>  	/* Check the blocks in the AGFL. */
> -	return xfs_scrub_walk_agfl(sc, xfs_scrub_agfl_block, NULL);
> +	error = xfs_scrub_walk_agfl(sc, xfs_scrub_agfl_block, &sai);
> +	if (error)
> +		goto out_free;
> +
> +	/* Sort entries, check for duplicates. */
> +	sort(sai.entries, sai.nr_entries, sizeof(sai.entries[0]),
> +			xfs_scrub_agblock_cmp, NULL);
> +	for (i = 1; i < sai.nr_entries; i++) {
> +		if (sai.entries[i] == sai.entries[i - 1]) {
> +			xfs_scrub_block_set_corrupt(sc, sc->sa.agf_bp);
> +			break;
> +		}
> +	}
> +
> +out_free:
> +	kmem_free(sai.entries);
>  out:
>  	return error;
>  }
> --
> 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 7/5] xfs: on failed mount, force-reclaim inodes after unmounting quota controls

[Dave Chinner]

On Wed, Nov 08, 2017 at 10:00:10PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> When mounting fails, we must force-reclaim inodes (and disable delayed
> reclaim) /after/ the realtime and quota control have let go of the
> realtime and quota inodes.  Without this, we corrupt the timer list and
> cause other weird problems.
> 
> Found by xfs/376 fuzzing u3.bmbt[0].lastoff on an rmap filesystem to
> force a bogus post-eof extent reclaim that causes the fs to go down.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/xfs_mount.c |    9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index e9727d0..a9f0fe9 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -1022,10 +1022,15 @@ xfs_mountfs(
>  	xfs_rtunmount_inodes(mp);
>   out_rele_rip:
>  	IRELE(rip);
> -	cancel_delayed_work_sync(&mp->m_reclaim_work);
> -	xfs_reclaim_inodes(mp, SYNC_WAIT);
>  	/* Clean out dquots that might be in memory after quotacheck. */
>  	xfs_qm_unmount(mp);
> +	/*
> +	 * Cancel all delayed reclaim work and reclaim the inodes directly.
> +	 * We have to do this /after/ rtunmount and qm_unmount because those
> +	 * two will have scheduled delayed reclaim for the rt/quota inodes.
> +	 */
> +	cancel_delayed_work_sync(&mp->m_reclaim_work);
> +	xfs_reclaim_inodes(mp, SYNC_WAIT);
>   out_log_dealloc:
>  	mp->m_flags |= XFS_MOUNT_UNMOUNTING;
>  	xfs_log_mount_cancel(mp);

Same bug in xfs_unmountfs(), isn't there? Otherwise this needs
explaining why the order is different to a normal unmount...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 8/5] xfs: remove u_int* type usage

[Dave Chinner]

On Thu, Nov 09, 2017 at 09:35:47AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Use the uint* types instead of the u_int* types.  This will (hopefully)
> pair with an xfsprogs cleanup.
> 
> 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 v3 1/5] xfs: check the uniqueness of the AGFL entries

[Darrick J. Wong]

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

Make sure we don't list a block twice in the agfl by copying the
contents of the AGFL to an array, sorting it, and looking for
duplicates.  We can easily check that the number of agfl entries we see
actually matches the flcount, so do that too.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
v3: check flcount, don't overflow buffer
v2: minor reworks per dchinner review suggestions
---
 fs/xfs/scrub/agheader.c |   64 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 62 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/scrub/agheader.c b/fs/xfs/scrub/agheader.c
index 5495aa5..f9d3c50 100644
--- a/fs/xfs/scrub/agheader.c
+++ b/fs/xfs/scrub/agheader.c
@@ -476,6 +476,12 @@ xfs_scrub_agf(
 
 /* AGFL */
 
+struct xfs_scrub_agfl_info {
+	unsigned int			sz_entries;
+	unsigned int			nr_entries;
+	xfs_agblock_t			*entries;
+};
+
 /* Scrub an AGFL block. */
 STATIC int
 xfs_scrub_agfl_block(
@@ -484,20 +490,39 @@ xfs_scrub_agfl_block(
 	void				*priv)
 {
 	struct xfs_mount		*mp = sc->mp;
+	struct xfs_scrub_agfl_info	*sai = priv;
 	xfs_agnumber_t			agno = sc->sa.agno;
 
-	if (!xfs_verify_agbno(mp, agno, agbno))
+	if (xfs_verify_agbno(mp, agno, agbno) &&
+	    sai->nr_entries < sai->sz_entries)
+		sai->entries[sai->nr_entries++] = agbno;
+	else
 		xfs_scrub_block_set_corrupt(sc, sc->sa.agfl_bp);
 
 	return 0;
 }
 
+static int
+xfs_scrub_agblock_cmp(
+	const void		*pa,
+	const void		*pb)
+{
+	const xfs_agblock_t	*a = pa;
+	const xfs_agblock_t	*b = pb;
+
+	return (int)*a - (int)*b;
+}
+
 /* Scrub the AGFL. */
 int
 xfs_scrub_agfl(
 	struct xfs_scrub_context	*sc)
 {
+	struct xfs_scrub_agfl_info	sai = { 0 };
+	struct xfs_agf			*agf;
 	xfs_agnumber_t			agno;
+	unsigned int			agflcount;
+	unsigned int			i;
 	int				error;
 
 	agno = sc->sa.agno = sc->sm->sm_agno;
@@ -508,8 +533,43 @@ xfs_scrub_agfl(
 	if (!sc->sa.agf_bp)
 		return -EFSCORRUPTED;
 
+	/* Allocate buffer to ensure uniqueness of AGFL entries. */
+	agf = XFS_BUF_TO_AGF(sc->sa.agf_bp);
+	agflcount = be32_to_cpu(agf->agf_flcount);
+	if (agflcount > XFS_AGFL_SIZE(sc->mp)) {
+		xfs_scrub_block_set_corrupt(sc, sc->sa.agf_bp);
+		goto out;
+	}
+	sai.sz_entries = agflcount;
+	sai.entries = kmem_zalloc(sizeof(xfs_agblock_t) * agflcount,
+			KM_SLEEP | KM_NOFS);
+	if (!sai.entries) {
+		error = -ENOMEM;
+		goto out;
+	}
+
 	/* Check the blocks in the AGFL. */
-	return xfs_scrub_walk_agfl(sc, xfs_scrub_agfl_block, NULL);
+	error = xfs_scrub_walk_agfl(sc, xfs_scrub_agfl_block, &sai);
+	if (error)
+		goto out_free;
+
+	if (agflcount != sai.nr_entries) {
+		xfs_scrub_block_set_corrupt(sc, sc->sa.agf_bp);
+		goto out_free;
+	}
+
+	/* Sort entries, check for duplicates. */
+	sort(sai.entries, sai.nr_entries, sizeof(sai.entries[0]),
+			xfs_scrub_agblock_cmp, NULL);
+	for (i = 1; i < sai.nr_entries; i++) {
+		if (sai.entries[i] == sai.entries[i - 1]) {
+			xfs_scrub_block_set_corrupt(sc, sc->sa.agf_bp);
+			break;
+		}
+	}
+
+out_free:
+	kmem_free(sai.entries);
 out:
 	return error;
 }

Re: [PATCH 7/5] xfs: on failed mount, force-reclaim inodes after unmounting quota controls

[Darrick J. Wong]

On Fri, Nov 10, 2017 at 10:24:25AM +1100, Dave Chinner wrote:
> On Wed, Nov 08, 2017 at 10:00:10PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > When mounting fails, we must force-reclaim inodes (and disable delayed
> > reclaim) /after/ the realtime and quota control have let go of the
> > realtime and quota inodes.  Without this, we corrupt the timer list and
> > cause other weird problems.
> > 
> > Found by xfs/376 fuzzing u3.bmbt[0].lastoff on an rmap filesystem to
> > force a bogus post-eof extent reclaim that causes the fs to go down.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/xfs_mount.c |    9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> > index e9727d0..a9f0fe9 100644
> > --- a/fs/xfs/xfs_mount.c
> > +++ b/fs/xfs/xfs_mount.c
> > @@ -1022,10 +1022,15 @@ xfs_mountfs(
> >  	xfs_rtunmount_inodes(mp);
> >   out_rele_rip:
> >  	IRELE(rip);
> > -	cancel_delayed_work_sync(&mp->m_reclaim_work);
> > -	xfs_reclaim_inodes(mp, SYNC_WAIT);
> >  	/* Clean out dquots that might be in memory after quotacheck. */
> >  	xfs_qm_unmount(mp);
> > +	/*
> > +	 * Cancel all delayed reclaim work and reclaim the inodes directly.
> > +	 * We have to do this /after/ rtunmount and qm_unmount because those
> > +	 * two will have scheduled delayed reclaim for the rt/quota inodes.
> > +	 */
> > +	cancel_delayed_work_sync(&mp->m_reclaim_work);
> > +	xfs_reclaim_inodes(mp, SYNC_WAIT);
> >   out_log_dealloc:
> >  	mp->m_flags |= XFS_MOUNT_UNMOUNTING;
> >  	xfs_log_mount_cancel(mp);
> 
> Same bug in xfs_unmountfs(), isn't there? Otherwise this needs
> explaining why the order is different to a normal unmount...

Slightly different circumstances here.  In the unmountfs path we're
guaranteed to have called xfs_qm_unmount_quotas (which will irele the
quota inodes) before we shut down inode reclaim, but in the mountfs
error-out paths we decide to bail out after xfs_qm_newmount but before
calling xfs_qm_mount_quotas (e.g.  log_mount_finish failure), which
means that xfs_qm_unmount is the only chance we have to clean out the
quota inodes.

--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 v2 7/5] xfs: on failed mount, force-reclaim inodes after unmounting quota controls

[Darrick J. Wong]

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

When mounting fails, we must force-reclaim inodes (and disable delayed
reclaim) /after/ the realtime and quota control have let go of the
realtime and quota inodes.  Without this, we corrupt the timer list and
cause other weird problems.

Found by xfs/376 fuzzing u3.bmbt[0].lastoff on an rmap filesystem to
force a bogus post-eof extent reclaim that causes the fs to go down.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
v2: try again with longer comment
---
 fs/xfs/xfs_mount.c |   15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index e9727d0..c879b51 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -1022,10 +1022,21 @@ xfs_mountfs(
 	xfs_rtunmount_inodes(mp);
  out_rele_rip:
 	IRELE(rip);
-	cancel_delayed_work_sync(&mp->m_reclaim_work);
-	xfs_reclaim_inodes(mp, SYNC_WAIT);
 	/* Clean out dquots that might be in memory after quotacheck. */
 	xfs_qm_unmount(mp);
+	/*
+	 * Cancel all delayed reclaim work and reclaim the inodes directly.
+	 * We have to do this /after/ rtunmount and qm_unmount because those
+	 * two will have scheduled delayed reclaim for the rt/quota inodes.
+	 *
+	 * This is slightly different from the unmountfs call sequence
+	 * because we could be tearing down a partially set up mount.  In
+	 * particular, if log_mount_finish fails we bail out without calling
+	 * qm_unmount_quotas and therefore rely on qm_unmount to release the
+	 * quota inodes.
+	 */
+	cancel_delayed_work_sync(&mp->m_reclaim_work);
+	xfs_reclaim_inodes(mp, SYNC_WAIT);
  out_log_dealloc:
 	mp->m_flags |= XFS_MOUNT_UNMOUNTING;
 	xfs_log_mount_cancel(mp);

Re: [PATCH v2 7/5] xfs: on failed mount, force-reclaim inodes after unmounting quota controls

[Dave Chinner]

On Thu, Nov 09, 2017 at 03:57:48PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> When mounting fails, we must force-reclaim inodes (and disable delayed
> reclaim) /after/ the realtime and quota control have let go of the
> realtime and quota inodes.  Without this, we corrupt the timer list and
> cause other weird problems.
> 
> Found by xfs/376 fuzzing u3.bmbt[0].lastoff on an rmap filesystem to
> force a bogus post-eof extent reclaim that causes the fs to go down.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> v2: try again with longer comment
> ---
>  fs/xfs/xfs_mount.c |   15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index e9727d0..c879b51 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -1022,10 +1022,21 @@ xfs_mountfs(
>  	xfs_rtunmount_inodes(mp);
>   out_rele_rip:
>  	IRELE(rip);
> -	cancel_delayed_work_sync(&mp->m_reclaim_work);
> -	xfs_reclaim_inodes(mp, SYNC_WAIT);
>  	/* Clean out dquots that might be in memory after quotacheck. */
>  	xfs_qm_unmount(mp);
> +	/*
> +	 * Cancel all delayed reclaim work and reclaim the inodes directly.
> +	 * We have to do this /after/ rtunmount and qm_unmount because those
> +	 * two will have scheduled delayed reclaim for the rt/quota inodes.
> +	 *
> +	 * This is slightly different from the unmountfs call sequence
> +	 * because we could be tearing down a partially set up mount.  In
> +	 * particular, if log_mount_finish fails we bail out without calling
> +	 * qm_unmount_quotas and therefore rely on qm_unmount to release the
> +	 * quota inodes.
> +	 */
> +	cancel_delayed_work_sync(&mp->m_reclaim_work);
> +	xfs_reclaim_inodes(mp, SYNC_WAIT);

Yup, that's better - I know what is going on now and I don't have to
remember the details. Double win! :P

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


-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v3 1/5] xfs: check the uniqueness of the AGFL entries

[Dave Chinner]

On Thu, Nov 09, 2017 at 03:39:44PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Make sure we don't list a block twice in the agfl by copying the
> contents of the AGFL to an array, sorting it, and looking for
> duplicates.  We can easily check that the number of agfl entries we see
> actually matches the flcount, so do that too.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> v3: check flcount, don't overflow buffer
> v2: minor reworks per dchinner review suggestions

Looks good. One minor nit:

> +	sai.entries = kmem_zalloc(sizeof(xfs_agblock_t) * agflcount,
> +			KM_SLEEP | KM_NOFS);
> +	if (!sai.entries) {
> +		error = -ENOMEM;
> +		goto out;
> +	}

KM_SLEEP means "never fail", which would make either it or the error
checking redundant. I'd just drop the KM_SLEEP - it's not necessary
if we can handle ENOMEM effectively.

Otherwise it looks good.

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

-- 
Dave Chinner
david@fromorbit.com