[PATCH 0/6] xfs: various coverity fixes

[PATCH 0/6] xfs: various coverity fixes

[Darrick J. Wong]

Hi all,

In a previous series we performed a lot of cleanups to the metadata
corruption detection code in XFS.  Now add the ability to report these
problems to the health tracking system so that administrators can gather
reports on live filesystems, and (in the future) to enable more targeted
scanning of metadata.

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

--D

[PATCH 1/6] 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      |    1 +
 fs/xfs/xfs_log_priv.h |    5 ++++-
 2 files changed, 5 insertions(+), 1 deletion(-)


diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index d7d3bfd6a920..1b4e37bbce53 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -2808,6 +2808,7 @@ 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..78fef8fc18b3 100644
--- a/fs/xfs/xfs_log_priv.h
+++ b/fs/xfs/xfs_log_priv.h
@@ -537,7 +537,10 @@ 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);
 

[PATCH 2/6] xfs: fix missing header includes

[Darrick J. Wong]

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

Some of the xfs source files are missing header includes, so add them
back.  Sparse complains about non-static functions that don't have a
forward declaration anywhere.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_ag_resv.c     |    2 ++
 fs/xfs/libxfs/xfs_attr_remote.c |    1 +
 fs/xfs/libxfs/xfs_bit.c         |    1 +
 fs/xfs/libxfs/xfs_sb.c          |    1 +
 fs/xfs/scrub/health.c           |    1 +
 fs/xfs/scrub/scrub.c            |    1 +
 fs/xfs/xfs_acl.c                |    3 ++-
 fs/xfs/xfs_discard.c            |    1 +
 fs/xfs/xfs_ioctl.c              |    1 +
 fs/xfs/xfs_symlink.c            |    1 +
 fs/xfs/xfs_xattr.c              |    1 +
 11 files changed, 13 insertions(+), 1 deletion(-)


diff --git a/fs/xfs/libxfs/xfs_ag_resv.c b/fs/xfs/libxfs/xfs_ag_resv.c
index 87a9747f1d36..fdfe6dc0d307 100644
--- a/fs/xfs/libxfs/xfs_ag_resv.c
+++ b/fs/xfs/libxfs/xfs_ag_resv.c
@@ -19,6 +19,8 @@
 #include "xfs_btree.h"
 #include "xfs_refcount_btree.h"
 #include "xfs_ialloc_btree.h"
+#include "xfs_sb.h"
+#include "xfs_ag_resv.h"
 
 /*
  * Per-AG Block Reservations
diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
index 3e39b7d40f25..a6ef5df42669 100644
--- a/fs/xfs/libxfs/xfs_attr_remote.c
+++ b/fs/xfs/libxfs/xfs_attr_remote.c
@@ -19,6 +19,7 @@
 #include "xfs_trans.h"
 #include "xfs_bmap.h"
 #include "xfs_attr.h"
+#include "xfs_attr_remote.h"
 #include "xfs_trace.h"
 #include "xfs_error.h"
 
diff --git a/fs/xfs/libxfs/xfs_bit.c b/fs/xfs/libxfs/xfs_bit.c
index 7071ff98fdbc..40ce5f3094d1 100644
--- a/fs/xfs/libxfs/xfs_bit.c
+++ b/fs/xfs/libxfs/xfs_bit.c
@@ -5,6 +5,7 @@
  */
 #include "xfs.h"
 #include "xfs_log_format.h"
+#include "xfs_bit.h"
 
 /*
  * XFS bit manipulation routines, used in non-realtime code.
diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
index ac6cdca63e15..0ac69751fe85 100644
--- a/fs/xfs/libxfs/xfs_sb.c
+++ b/fs/xfs/libxfs/xfs_sb.c
@@ -10,6 +10,7 @@
 #include "xfs_log_format.h"
 #include "xfs_trans_resv.h"
 #include "xfs_bit.h"
+#include "xfs_sb.h"
 #include "xfs_mount.h"
 #include "xfs_ialloc.h"
 #include "xfs_alloc.h"
diff --git a/fs/xfs/scrub/health.c b/fs/xfs/scrub/health.c
index b2f602811e9d..83d27cdf579b 100644
--- a/fs/xfs/scrub/health.c
+++ b/fs/xfs/scrub/health.c
@@ -11,6 +11,7 @@
 #include "xfs_sb.h"
 #include "xfs_health.h"
 #include "scrub/scrub.h"
+#include "scrub/health.h"
 
 /*
  * Scrub and In-Core Filesystem Health Assessments
diff --git a/fs/xfs/scrub/scrub.c b/fs/xfs/scrub/scrub.c
index 15c8c5f3f688..f1775bb19313 100644
--- a/fs/xfs/scrub/scrub.c
+++ b/fs/xfs/scrub/scrub.c
@@ -16,6 +16,7 @@
 #include "xfs_qm.h"
 #include "xfs_errortag.h"
 #include "xfs_error.h"
+#include "xfs_scrub.h"
 #include "scrub/scrub.h"
 #include "scrub/common.h"
 #include "scrub/trace.h"
diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
index 3f2292c7835c..91693fce34a8 100644
--- a/fs/xfs/xfs_acl.c
+++ b/fs/xfs/xfs_acl.c
@@ -13,8 +13,9 @@
 #include "xfs_attr.h"
 #include "xfs_trace.h"
 #include "xfs_error.h"
-#include <linux/posix_acl_xattr.h>
+#include "xfs_acl.h"
 
+#include <linux/posix_acl_xattr.h>
 
 /*
  * Locking scheme:
diff --git a/fs/xfs/xfs_discard.c b/fs/xfs/xfs_discard.c
index 8ec7aab89044..50966a9912cd 100644
--- a/fs/xfs/xfs_discard.c
+++ b/fs/xfs/xfs_discard.c
@@ -13,6 +13,7 @@
 #include "xfs_btree.h"
 #include "xfs_alloc_btree.h"
 #include "xfs_alloc.h"
+#include "xfs_discard.h"
 #include "xfs_error.h"
 #include "xfs_extent_busy.h"
 #include "xfs_trace.h"
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 800f07044636..364961c23cd0 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -34,6 +34,7 @@
 #include "xfs_ag.h"
 #include "xfs_health.h"
 #include "xfs_reflink.h"
+#include "xfs_ioctl.h"
 
 #include <linux/mount.h>
 #include <linux/namei.h>
diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
index ed66fd2de327..a25502bc2071 100644
--- a/fs/xfs/xfs_symlink.c
+++ b/fs/xfs/xfs_symlink.c
@@ -17,6 +17,7 @@
 #include "xfs_bmap.h"
 #include "xfs_bmap_btree.h"
 #include "xfs_quota.h"
+#include "xfs_symlink.h"
 #include "xfs_trans_space.h"
 #include "xfs_trace.h"
 #include "xfs_trans.h"
diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c
index cb895b1df5e4..383f0203d103 100644
--- a/fs/xfs/xfs_xattr.c
+++ b/fs/xfs/xfs_xattr.c
@@ -11,6 +11,7 @@
 #include "xfs_da_format.h"
 #include "xfs_inode.h"
 #include "xfs_attr.h"
+#include "xfs_acl.h"
 
 #include <linux/posix_acl_xattr.h>
 #include <linux/xattr.h>

[PATCH 3/6] 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 |    6 +++++-
 fs/xfs/libxfs/xfs_btree.c |   17 +++++++++++------
 fs/xfs/libxfs/xfs_btree.h |    5 +++--
 3 files changed, 19 insertions(+), 9 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index f7a4b54c5bc2..f609e1ab14d4 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -1479,6 +1479,7 @@ xfs_alloc_ag_vextent_near(
 	int			i;		/* result code, temporary */
 	xfs_agblock_t		bno;
 	xfs_extlen_t		len;
+	bool			is_last;
 #ifdef DEBUG
 	/*
 	 * Randomly don't execute the first algorithm.
@@ -1532,7 +1533,10 @@ xfs_alloc_ag_vextent_near(
 	 * 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)) {
+	error = xfs_btree_islastblock(acur.cnt, 0, &is_last);
+	if (error)
+		goto out;
+	while (is_last) {
 #ifdef DEBUG
 		if (dofirst)
 			break;
diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
index 98843f1258b8..098abfb50252 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 */
+	int			level,	/* level to check */
+	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..ff54e6c18c44 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 */
+	int			level,	/* level to check */
+	bool			*last);
 
 /*
  * Compute first and last byte offsets for the fields given.

[PATCH 4/6] xfs: null out bma->prev if no previous extent

[Darrick J. Wong]

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

Coverity complains that we don't check the return value of
xfs_iext_peek_prev_extent like we do nearly all of the time.  If there
is no previous extent then just null out bma->prev like we do elsewhere
in the bmap code.

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


diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 64f623d07f82..7392ca92ab34 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -4014,7 +4014,8 @@ xfs_bmapi_allocate(
 	if (bma->wasdel) {
 		bma->length = (xfs_extlen_t)bma->got.br_blockcount;
 		bma->offset = bma->got.br_startoff;
-		xfs_iext_peek_prev_extent(ifp, &bma->icur, &bma->prev);
+		if (!xfs_iext_peek_prev_extent(ifp, &bma->icur, &bma->prev))
+			bma->prev.br_startoff = NULLFILEOFF;
 	} else {
 		bma->length = XFS_FILBLKS_MIN(bma->length, MAXEXTLEN);
 		if (!bma->eof)

[PATCH 5/6] xfs: "optimize" buffer item log segment bitmap setting

[Darrick J. Wong]

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

Optimize the setting of full words of bits in xfs_buf_item_log_segment.
The optimization is purely within the bug triage process.  No functional
changes.

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


diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index d74fbd1e9d3e..6b69e6137b2b 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -851,7 +851,7 @@ xfs_buf_item_log_segment(
 	 * first_bit and last_bit.
 	 */
 	while ((bits_to_set - bits_set) >= NBWORD) {
-		*wordp |= 0xffffffff;
+		*wordp = 0xffffffff;
 		bits_set += NBWORD;
 		wordp++;
 	}

[PATCH 6/6] xfs: range check ri_cnt when recovering log items

[Darrick J. Wong]

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

Range check the region counter when we're reassembling regions from log
items during log recovery.  In the old days ASSERT would halt the
kernel, but this isn't true any more so we have to make an explicit
error return.

Coverity-id: 1132508
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_log_recover.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)


diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 648d5ecafd91..b0257ef9d29f 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -4301,7 +4301,16 @@ xlog_recover_add_to_trans(
 			kmem_zalloc(item->ri_total * sizeof(xfs_log_iovec_t),
 				    0);
 	}
-	ASSERT(item->ri_total > item->ri_cnt);
+
+	if (item->ri_total <= item->ri_cnt) {
+		xfs_warn(log->l_mp,
+	"log item region count (%d) overflowed size (%d)",
+				item->ri_cnt, item->ri_total);
+		ASSERT(0);
+		kmem_free(ptr);
+		return -EFSCORRUPTED;
+	}
+
 	/* Description region is ri_buf[0] */
 	item->ri_buf[item->ri_cnt].i_addr = ptr;
 	item->ri_buf[item->ri_cnt].i_len  = len;

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

[Christoph Hellwig]

On Wed, Nov 06, 2019 at 07:02:25PM -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>
> ---
>  fs/xfs/xfs_log.c      |    1 +
>  fs/xfs/xfs_log_priv.h |    5 ++++-
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index d7d3bfd6a920..1b4e37bbce53 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2808,6 +2808,7 @@ xlog_state_do_iclog_callbacks(
>  	struct xlog		*log,
>  	struct xlog_in_core	*iclog,
>  	bool			aborted)
> +	__releases(&log->l_icloglock) __acquires(&log->l_icloglock)

The indentation looks really awkward.  I think this should be be:

	bool                    aborted)
		__releases(&log->l_icloglock)
		__acquires(&log->l_icloglock)

> +static inline void
> +xlog_wait(
> +	struct wait_queue_head	*wq,
> +	struct spinlock		*lock) __releases(lock)
>  {
>  	DECLARE_WAITQUEUE(wait, current);

Same here.

Re: [PATCH 2/6] xfs: fix missing header includes

[Christoph Hellwig]

Looks fine,

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

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

[Christoph Hellwig]

> -	while (xfs_btree_islastblock(acur.cnt, 0)) {
> +	error = xfs_btree_islastblock(acur.cnt, 0, &is_last);
> +	if (error)
> +		goto out;
> +	while (is_last) {

This transformation looks actually ok, but is highly non-obvious.
I think you want a prep patch just killing the pointless while first.

Re: [PATCH 4/6] xfs: null out bma->prev if no previous extent

[Christoph Hellwig]

On Wed, Nov 06, 2019 at 07:02:49PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Coverity complains that we don't check the return value of
> xfs_iext_peek_prev_extent like we do nearly all of the time.  If there
> is no previous extent then just null out bma->prev like we do elsewhere
> in the bmap code.

Looks fine,

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

Re: [PATCH 5/6] xfs: "optimize" buffer item log segment bitmap setting

[Christoph Hellwig]

On Wed, Nov 06, 2019 at 07:02:55PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Optimize the setting of full words of bits in xfs_buf_item_log_segment.
> The optimization is purely within the bug triage process.  No functional
> changes.

Looks good,

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

Re: [PATCH 6/6] xfs: range check ri_cnt when recovering log items

[Christoph Hellwig]

On Wed, Nov 06, 2019 at 07:03:01PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Range check the region counter when we're reassembling regions from log
> items during log recovery.  In the old days ASSERT would halt the
> kernel, but this isn't true any more so we have to make an explicit
> error return.

Looks good,

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

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

[Darrick J. Wong]

On Thu, Nov 07, 2019 at 12:31:58AM -0800, Christoph Hellwig wrote:
> On Wed, Nov 06, 2019 at 07:02:25PM -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>
> > ---
> >  fs/xfs/xfs_log.c      |    1 +
> >  fs/xfs/xfs_log_priv.h |    5 ++++-
> >  2 files changed, 5 insertions(+), 1 deletion(-)
> > 
> > 
> > diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> > index d7d3bfd6a920..1b4e37bbce53 100644
> > --- a/fs/xfs/xfs_log.c
> > +++ b/fs/xfs/xfs_log.c
> > @@ -2808,6 +2808,7 @@ xlog_state_do_iclog_callbacks(
> >  	struct xlog		*log,
> >  	struct xlog_in_core	*iclog,
> >  	bool			aborted)
> > +	__releases(&log->l_icloglock) __acquires(&log->l_icloglock)
> 
> The indentation looks really awkward.  I think this should be be:
> 
> 	bool                    aborted)
> 		__releases(&log->l_icloglock)
> 		__acquires(&log->l_icloglock)
> 
> > +static inline void
> > +xlog_wait(
> > +	struct wait_queue_head	*wq,
> > +	struct spinlock		*lock) __releases(lock)
> >  {
> >  	DECLARE_WAITQUEUE(wait, current);
> 
> Same here.

Will change both.

(I find both awkward, but not enough to push back all that hard. :P)

--D

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

[Darrick J. Wong]

On Thu, Nov 07, 2019 at 12:34:50AM -0800, Christoph Hellwig wrote:
> > -	while (xfs_btree_islastblock(acur.cnt, 0)) {
> > +	error = xfs_btree_islastblock(acur.cnt, 0, &is_last);
> > +	if (error)
> > +		goto out;
> > +	while (is_last) {
> 
> This transformation looks actually ok, but is highly non-obvious.
> I think you want a prep patch just killing the pointless while first.

Yeah, killing the while was a little more involved than I thought it
would be, but it's done.

--D