[PATCH v2 0/3] xfs: various coverity fixes

[PATCH v2 0/3] xfs: various coverity fixes

[Darrick J. Wong]

Hi all,

This series fixes a few things that Coverity has complained about for
years.

This has been lightly tested with fstests.  Enjoy!
Comments and questions are, as always, welcome.

--D

[PATCH 1/3] xfs: annotate functions that trip static checker locking checks

[Darrick J. Wong]

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

Add some lock annotations to helper functions that seem to have
unbalanced locking that confuses the static analyzers.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_log.c      |    2 ++
 fs/xfs/xfs_log_priv.h |    6 +++++-
 2 files changed, 7 insertions(+), 1 deletion(-)


diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index d7d3bfd6a920..3806674090ed 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -2808,6 +2808,8 @@ xlog_state_do_iclog_callbacks(
 	struct xlog		*log,
 	struct xlog_in_core	*iclog,
 	bool			aborted)
+		__releases(&log->l_icloglock)
+		__acquires(&log->l_icloglock)
 {
 	spin_unlock(&log->l_icloglock);
 	spin_lock(&iclog->ic_callback_lock);
diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
index 4f19375f6592..c47aa2ca6dc7 100644
--- a/fs/xfs/xfs_log_priv.h
+++ b/fs/xfs/xfs_log_priv.h
@@ -537,7 +537,11 @@ xlog_cil_force(struct xlog *log)
  * by a spinlock. This matches the semantics of all the wait queues used in the
  * log code.
  */
-static inline void xlog_wait(wait_queue_head_t *wq, spinlock_t *lock)
+static inline void
+xlog_wait(
+	struct wait_queue_head	*wq,
+	struct spinlock		*lock)
+		__releases(lock)
 {
 	DECLARE_WAITQUEUE(wait, current);
 

Re: [PATCH 1/3] xfs: annotate functions that trip static checker locking checks

[Christoph Hellwig]

On Thu, Nov 07, 2019 at 11:04:51PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Add some lock annotations to helper functions that seem to have
> unbalanced locking that confuses the static analyzers.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

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

[PATCH 2/3] xfs: clean up weird while loop in xfs_alloc_ag_vextent_near

[Darrick J. Wong]

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

Refactor the weird while loop out of existence.

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


diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index 60ca09bedc23..3a724d80783a 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -1486,6 +1486,65 @@ xfs_alloc_ag_vextent_locality(
 	return 0;
 }
 
+/* Check the last block of the cnt btree for allocations. */
+static int
+xfs_alloc_ag_vextent_lastblock(
+	struct xfs_alloc_arg	*args,
+	struct xfs_alloc_cur	*acur,
+	xfs_agblock_t		*bno,
+	xfs_extlen_t		*len,
+	bool			*allocated)
+{
+	int			error;
+	int			i;
+
+#ifdef DEBUG
+	/* Randomly don't execute the first algorithm. */
+	if (prandom_u32() & 1)
+		return 0;
+#endif
+
+	/*
+	 * Start from the entry that lookup found, sequence through all larger
+	 * free blocks.  If we're actually pointing at a record smaller than
+	 * maxlen, go to the start of this block, and skip all those smaller
+	 * than minlen.
+	 */
+	if (len || args->alignment > 1) {
+		acur->cnt->bc_ptrs[0] = 1;
+		do {
+			error = xfs_alloc_get_rec(acur->cnt, bno, len, &i);
+			if (error)
+				return error;
+			if (XFS_IS_CORRUPT(args->mp, i != 1))
+				return -EFSCORRUPTED;
+			if (*len >= args->minlen)
+				break;
+			error = xfs_btree_increment(acur->cnt, 0, &i);
+			if (error)
+				return error;
+		} while (i);
+		ASSERT(*len >= args->minlen);
+		if (!i)
+			return 0;
+	}
+
+	error = xfs_alloc_walk_iter(args, acur, acur->cnt, true, false, -1, &i);
+	if (error)
+		return error;
+
+	/*
+	 * It didn't work.  We COULD be in a case where there's a good record
+	 * somewhere, so try again.
+	 */
+	if (acur->len == 0)
+		return 0;
+
+	trace_xfs_alloc_near_first(args);
+	*allocated = true;
+	return 0;
+}
+
 /*
  * Allocate a variable extent near bno in the allocation group agno.
  * Extent's length (returned in len) will be between minlen and maxlen,
@@ -1501,14 +1560,6 @@ xfs_alloc_ag_vextent_near(
 	int			i;		/* result code, temporary */
 	xfs_agblock_t		bno;
 	xfs_extlen_t		len;
-#ifdef DEBUG
-	/*
-	 * Randomly don't execute the first algorithm.
-	 */
-	int		dofirst;	/* set to do first algorithm */
-
-	dofirst = prandom_u32() & 1;
-#endif
 
 	/* handle uninitialized agbno range so caller doesn't have to */
 	if (!args->min_agbno && !args->max_agbno)
@@ -1551,54 +1602,16 @@ xfs_alloc_ag_vextent_near(
 	 * near the right edge of the tree.  If it's in the last btree leaf
 	 * block, then we just examine all the entries in that block
 	 * that are big enough, and pick the best one.
-	 * This is written as a while loop so we can break out of it,
-	 * but we never loop back to the top.
 	 */
-	while (xfs_btree_islastblock(acur.cnt, 0)) {
-#ifdef DEBUG
-		if (dofirst)
-			break;
-#endif
-		/*
-		 * Start from the entry that lookup found, sequence through
-		 * all larger free blocks.  If we're actually pointing at a
-		 * record smaller than maxlen, go to the start of this block,
-		 * and skip all those smaller than minlen.
-		 */
-		if (len || args->alignment > 1) {
-			acur.cnt->bc_ptrs[0] = 1;
-			do {
-				error = xfs_alloc_get_rec(acur.cnt, &bno, &len,
-						&i);
-				if (error)
-					goto out;
-				if (XFS_IS_CORRUPT(args->mp, i != 1))
-					goto out;
-				if (len >= args->minlen)
-					break;
-				error = xfs_btree_increment(acur.cnt, 0, &i);
-				if (error)
-					goto out;
-			} while (i);
-			ASSERT(len >= args->minlen);
-			if (!i)
-				break;
-		}
+	if (xfs_btree_islastblock(acur.cnt, 0)) {
+		bool		allocated = false;
 
-		error = xfs_alloc_walk_iter(args, &acur, acur.cnt, true, false,
-					    -1, &i);
+		error = xfs_alloc_ag_vextent_lastblock(args, &acur, &bno, &len,
+				&allocated);
 		if (error)
 			goto out;
-
-		/*
-		 * It didn't work.  We COULD be in a case where
-		 * there's a good record somewhere, so try again.
-		 */
-		if (acur.len == 0)
-			break;
-
-		trace_xfs_alloc_near_first(args);
-		goto alloc;
+		if (allocated)
+			goto alloc_finish;
 	}
 
 	/*
@@ -1624,7 +1637,7 @@ xfs_alloc_ag_vextent_near(
 		goto out;
 	}
 
-alloc:
+alloc_finish:
 	/* fix up btrees on a successful allocation */
 	error = xfs_alloc_cur_finish(args, &acur);
 

Re: [PATCH 2/3] xfs: clean up weird while loop in xfs_alloc_ag_vextent_near

[Christoph Hellwig]

What tree is this supposed to apply to?  It fails against
xfs/for-next here.  Otherwise this looks fine to me.

Re: [PATCH 2/3] xfs: clean up weird while loop in xfs_alloc_ag_vextent_near

[Darrick J. Wong]

On Thu, Nov 07, 2019 at 11:11:51PM -0800, Christoph Hellwig wrote:
> What tree is this supposed to apply to?  It fails against

Sorry, I've been reworking these patches against a branch that I hadn't
yet pushed to xfs-linux.git (and won't until I can take one more look
tomorrow with a fresh brain).

In the meantime, you can see my development branch on my personal git
repo:

https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/log/?h=xfs-5.5-dev

> xfs/for-next here.  Otherwise this looks fine to me.

Fine enough for an RVB provided the above? :D

--D

Re: [PATCH 2/3] xfs: clean up weird while loop in xfs_alloc_ag_vextent_near

[Christoph Hellwig]

On Thu, Nov 07, 2019 at 11:20:02PM -0800, Darrick J. Wong wrote:
> On Thu, Nov 07, 2019 at 11:11:51PM -0800, Christoph Hellwig wrote:
> > What tree is this supposed to apply to?  It fails against
> 
> Sorry, I've been reworking these patches against a branch that I hadn't
> yet pushed to xfs-linux.git (and won't until I can take one more look
> tomorrow with a fresh brain).
> 
> In the meantime, you can see my development branch on my personal git
> repo:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/log/?h=xfs-5.5-dev
> 
> > xfs/for-next here.  Otherwise this looks fine to me.
> 
> Fine enough for an RVB provided the above? :D

Sure, it _looks_ good:

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

Although I'd love to be able to at least do a quick xfstests run on
it..

[PATCH 3/3] xfs: actually check xfs_btree_check_block return in xfs_btree_islastblock

[Darrick J. Wong]

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

Coverity points out that xfs_btree_islastblock calls
xfs_btree_check_block, but doesn't act on an error return.  This
predicate has no answer if the btree is corrupt, so tweak the helper to
be able to return errors, and then modify the one call site.

Coverity-id: 114069
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_alloc.c |    7 ++++++-
 fs/xfs/libxfs/xfs_btree.c |   19 ++++++++++++-------
 fs/xfs/libxfs/xfs_btree.h |    7 ++++---
 3 files changed, 22 insertions(+), 11 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index 3a724d80783a..65d870189548 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -1560,6 +1560,7 @@ xfs_alloc_ag_vextent_near(
 	int			i;		/* result code, temporary */
 	xfs_agblock_t		bno;
 	xfs_extlen_t		len;
+	bool			is_last;
 
 	/* handle uninitialized agbno range so caller doesn't have to */
 	if (!args->min_agbno && !args->max_agbno)
@@ -1603,7 +1604,11 @@ xfs_alloc_ag_vextent_near(
 	 * block, then we just examine all the entries in that block
 	 * that are big enough, and pick the best one.
 	 */
-	if (xfs_btree_islastblock(acur.cnt, 0)) {
+	error = xfs_btree_islastblock(acur.cnt, 0, &is_last);
+	if (error)
+		goto out;
+
+	if (is_last) {
 		bool		allocated = false;
 
 		error = xfs_alloc_ag_vextent_lastblock(args, &acur, &bno, &len,
diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
index 8ad153bc4dea..37b4fa93085c 100644
--- a/fs/xfs/libxfs/xfs_btree.c
+++ b/fs/xfs/libxfs/xfs_btree.c
@@ -719,20 +719,25 @@ xfs_btree_get_bufs(
 /*
  * Check for the cursor referring to the last block at the given level.
  */
-int					/* 1=is last block, 0=not last block */
+int
 xfs_btree_islastblock(
-	xfs_btree_cur_t		*cur,	/* btree cursor */
-	int			level)	/* level to check */
+	xfs_btree_cur_t		*cur,
+	int			level,
+	bool			*last)
 {
 	struct xfs_btree_block	*block;	/* generic btree block pointer */
-	xfs_buf_t		*bp;	/* buffer containing block */
+	struct xfs_buf		*bp;	/* buffer containing block */
+	int			error;
 
 	block = xfs_btree_get_block(cur, level, &bp);
-	xfs_btree_check_block(cur, block, level, bp);
+	error = xfs_btree_check_block(cur, block, level, bp);
+	if (error)
+		return error;
 	if (cur->bc_flags & XFS_BTREE_LONG_PTRS)
-		return block->bb_u.l.bb_rightsib == cpu_to_be64(NULLFSBLOCK);
+		*last = block->bb_u.l.bb_rightsib == cpu_to_be64(NULLFSBLOCK);
 	else
-		return block->bb_u.s.bb_rightsib == cpu_to_be32(NULLAGBLOCK);
+		*last = block->bb_u.s.bb_rightsib == cpu_to_be32(NULLAGBLOCK);
+	return 0;
 }
 
 /*
diff --git a/fs/xfs/libxfs/xfs_btree.h b/fs/xfs/libxfs/xfs_btree.h
index 6670120cd690..c63da9dd05cc 100644
--- a/fs/xfs/libxfs/xfs_btree.h
+++ b/fs/xfs/libxfs/xfs_btree.h
@@ -320,10 +320,11 @@ xfs_btree_get_bufs(
 /*
  * Check for the cursor referring to the last block at the given level.
  */
-int					/* 1=is last block, 0=not last block */
+int
 xfs_btree_islastblock(
-	xfs_btree_cur_t		*cur,	/* btree cursor */
-	int			level);	/* level to check */
+	xfs_btree_cur_t		*cur,
+	int			level,
+	bool			*last);
 
 /*
  * Compute first and last byte offsets for the fields given.

Re: [PATCH 3/3] xfs: actually check xfs_btree_check_block return in xfs_btree_islastblock

[Christoph Hellwig]

On Thu, Nov 07, 2019 at 11:05:04PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Coverity points out that xfs_btree_islastblock calls
> xfs_btree_check_block, but doesn't act on an error return.  This
> predicate has no answer if the btree is corrupt, so tweak the helper to
> be able to return errors, and then modify the one call site.

Could we just kill xfs_btree_islastblock?  It has pretty trivial, and
only has a single caller which only uses on of the two branches in the
function anyway.

Re: [PATCH 3/3] xfs: actually check xfs_btree_check_block return in xfs_btree_islastblock

[Darrick J. Wong]

On Thu, Nov 07, 2019 at 11:14:41PM -0800, Christoph Hellwig wrote:
> On Thu, Nov 07, 2019 at 11:05:04PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Coverity points out that xfs_btree_islastblock calls
> > xfs_btree_check_block, but doesn't act on an error return.  This
> > predicate has no answer if the btree is corrupt, so tweak the helper to
> > be able to return errors, and then modify the one call site.
> 
> Could we just kill xfs_btree_islastblock?  It has pretty trivial, and
> only has a single caller which only uses on of the two branches in the
> function anyway.

I'd rather leave it as a btree primitive, honestly.

That said, "Is this cursor pointing to the last block on $level?" only
makes sense if you've already performed a lookup (or seek) operation.
If you've done that, you've already checked the block, right?  So I
think we could just get rid of the _check_block call on the grounds that
we already did that as part of the lookup (or turn it into an ASSERT),
and then this becomes a short enough function to try to make it a four
line static inline predicate.

Same result, but slightly better encapsulation.

(Yeah yeah, it's C, we're all one big happy family of bits...)

--D

Re: [PATCH 3/3] xfs: actually check xfs_btree_check_block return in xfs_btree_islastblock

[Christoph Hellwig]

On Thu, Nov 07, 2019 at 11:29:51PM -0800, Darrick J. Wong wrote:
> That said, "Is this cursor pointing to the last block on $level?" only
> makes sense if you've already performed a lookup (or seek) operation.
> If you've done that, you've already checked the block, right?  So I
> think we could just get rid of the _check_block call on the grounds that
> we already did that as part of the lookup (or turn it into an ASSERT),
> and then this becomes a short enough function to try to make it a four
> line static inline predicate.
> 
> Same result, but slightly better encapsulation.
> 
> (Yeah yeah, it's C, we're all one big happy family of bits...)

Sounds ok.

Re: [PATCH 3/3] xfs: actually check xfs_btree_check_block return in xfs_btree_islastblock

[Darrick J. Wong]

On Thu, Nov 07, 2019 at 11:33:06PM -0800, Christoph Hellwig wrote:
> On Thu, Nov 07, 2019 at 11:29:51PM -0800, Darrick J. Wong wrote:
> > That said, "Is this cursor pointing to the last block on $level?" only
> > makes sense if you've already performed a lookup (or seek) operation.
> > If you've done that, you've already checked the block, right?  So I
> > think we could just get rid of the _check_block call on the grounds that
> > we already did that as part of the lookup (or turn it into an ASSERT),
> > and then this becomes a short enough function to try to make it a four
> > line static inline predicate.
> > 
> > Same result, but slightly better encapsulation.
> > 
> > (Yeah yeah, it's C, we're all one big happy family of bits...)
> 
> Sounds ok.

Ok, I'll push out a (lightly tested) patch.

--D

[PATCH 3/3] xfs: actually check xfs_btree_check_block return in xfs_btree_islastblock

[Darrick J. Wong]

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

Coverity points out that xfs_btree_islastblock doesn't check the return
value of xfs_btree_check_block.  Since the question "Does the cursor
point to the last block in this level?" only makes sense if the caller
previously performed a lookup or seek operation, the block should
already have been checked.

Therefore, check the return value in an ASSERT and turn the whole thing
into a static inline predicate.

Coverity-id: 114069
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_btree.c |   19 -------------------
 fs/xfs/libxfs/xfs_btree.h |   25 +++++++++++++++++--------
 2 files changed, 17 insertions(+), 27 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
index 8ad153bc4dea..897dfb9e4682 100644
--- a/fs/xfs/libxfs/xfs_btree.c
+++ b/fs/xfs/libxfs/xfs_btree.c
@@ -716,25 +716,6 @@ xfs_btree_get_bufs(
 	return xfs_trans_get_buf(tp, mp->m_ddev_targp, d, mp->m_bsize, 0);
 }
 
-/*
- * Check for the cursor referring to the last block at the given level.
- */
-int					/* 1=is last block, 0=not last block */
-xfs_btree_islastblock(
-	xfs_btree_cur_t		*cur,	/* btree cursor */
-	int			level)	/* level to check */
-{
-	struct xfs_btree_block	*block;	/* generic btree block pointer */
-	xfs_buf_t		*bp;	/* buffer containing block */
-
-	block = xfs_btree_get_block(cur, level, &bp);
-	xfs_btree_check_block(cur, block, level, bp);
-	if (cur->bc_flags & XFS_BTREE_LONG_PTRS)
-		return block->bb_u.l.bb_rightsib == cpu_to_be64(NULLFSBLOCK);
-	else
-		return block->bb_u.s.bb_rightsib == cpu_to_be32(NULLAGBLOCK);
-}
-
 /*
  * Change the cursor to point to the first record at the given level.
  * Other levels are unaffected.
diff --git a/fs/xfs/libxfs/xfs_btree.h b/fs/xfs/libxfs/xfs_btree.h
index 6670120cd690..fb9b2121c628 100644
--- a/fs/xfs/libxfs/xfs_btree.h
+++ b/fs/xfs/libxfs/xfs_btree.h
@@ -317,14 +317,6 @@ xfs_btree_get_bufs(
 	xfs_agnumber_t		agno,	/* allocation group number */
 	xfs_agblock_t		agbno);	/* allocation group block number */
 
-/*
- * Check for the cursor referring to the last block at the given level.
- */
-int					/* 1=is last block, 0=not last block */
-xfs_btree_islastblock(
-	xfs_btree_cur_t		*cur,	/* btree cursor */
-	int			level);	/* level to check */
-
 /*
  * Compute first and last byte offsets for the fields given.
  * Interprets the offsets table, which contains struct field offsets.
@@ -524,4 +516,21 @@ int xfs_btree_has_record(struct xfs_btree_cur *cur, union xfs_btree_irec *low,
 		union xfs_btree_irec *high, bool *exists);
 bool xfs_btree_has_more_records(struct xfs_btree_cur *cur);
 
+/* Does this cursor point to the last block in the given level? */
+static inline bool
+xfs_btree_islastblock(
+	xfs_btree_cur_t		*cur,
+	int			level)
+{
+	struct xfs_btree_block	*block;
+	struct xfs_buf		*bp;
+
+	block = xfs_btree_get_block(cur, level, &bp);
+	ASSERT(block && xfs_btree_check_block(cur, block, level, bp) == 0);
+
+	if (cur->bc_flags & XFS_BTREE_LONG_PTRS)
+		return block->bb_u.l.bb_rightsib == cpu_to_be64(NULLFSBLOCK);
+	return block->bb_u.s.bb_rightsib == cpu_to_be32(NULLAGBLOCK);
+}
+
 #endif	/* __XFS_BTREE_H__ */

Re: [PATCH 3/3] xfs: actually check xfs_btree_check_block return in xfs_btree_islastblock

[Carlos Maiolino]

> +/* Does this cursor point to the last block in the given level? */

I think this comment is unnecessary tbh, the function's definition looks pretty
clear to me already.

> +static inline bool
> +xfs_btree_islastblock(
> +	xfs_btree_cur_t		*cur,
> +	int			level)
> +{
> +	struct xfs_btree_block	*block;
> +	struct xfs_buf		*bp;
> +
> +	block = xfs_btree_get_block(cur, level, &bp);

And here it might be useful for the future, to remind us why do we check it
inside an assertion, something like:

	/* Caller is already supposed to have done a lookup/seek op */
> +	ASSERT(block && xfs_btree_check_block(cur, block, level, bp) == 0);
> +

But, these are just nitpicks, and since I didn't see any review on this patch
yet, you can add with or without the suggestions above:

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

Cheers.

-- 
Carlos