[PATCH v2 0/2] xfs: always init fdblocks in mount and avoid f_bfree overflow

[PATCH v2 0/2] xfs: always init fdblocks in mount and avoid f_bfree overflow

[Zheng Bin]

v1->v2: modify comment, add is agf inited judge in xfs_ag_resv_init

Zheng Bin (2):
  xfs: always init fdblocks in mount
  xfs: avoid f_bfree overflow

 fs/xfs/libxfs/xfs_ag_resv.c | 11 +++++++----
 fs/xfs/xfs_mount.c          | 39 ++++++---------------------------------
 fs/xfs/xfs_super.c          |  3 ++-
 3 files changed, 15 insertions(+), 38 deletions(-)

--
2.7.4

[PATCH v2 1/2] xfs: always init fdblocks in mount

[Zheng Bin]

Use fuzz(hydra) to test XFS and automatically generate
tmp.img(XFS v5 format, but some metadata is wrong)

xfs_repair information(just one AG):
agf_freeblks 0, counted 3224 in ag 0
agf_longest 0, counted 3224 in ag 0
sb_fdblocks 3228, counted 3224

Test as follows:
mount tmp.img tmpdir
cp file1M tmpdir
sync

In 4.19-stable, sync will stuck, while in linux-next, sync not stuck.
The reason is same to commit d0c7feaf8767
("xfs: add agf freeblocks verify in xfs_agf_verify"), cause agf_longest
is 0, we can not block this in xfs_agf_verify.

Make sure fdblocks is always inited in mount(also init ifree, icount).

xfs_mountfs
  xfs_check_summary_counts
    xfs_initialize_perag_data

Signed-off-by: Zheng Bin <zhengbin13@huawei.com>
---
 fs/xfs/libxfs/xfs_ag_resv.c | 11 +++++++----
 fs/xfs/xfs_mount.c          | 33 ---------------------------------
 2 files changed, 7 insertions(+), 37 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_ag_resv.c b/fs/xfs/libxfs/xfs_ag_resv.c
index fdfe6dc..487961b 100644
--- a/fs/xfs/libxfs/xfs_ag_resv.c
+++ b/fs/xfs/libxfs/xfs_ag_resv.c
@@ -303,10 +303,13 @@ xfs_ag_resv_init(
 	}

 #ifdef DEBUG
-	/* need to read in the AGF for the ASSERT below to work */
-	error = xfs_alloc_pagf_init(pag->pag_mount, tp, pag->pag_agno, 0);
-	if (error)
-		return error;
+	if (!pag->pagf_init) {
+		/* need to read in the AGF for the ASSERT below to work */
+		error = xfs_alloc_pagf_init(pag->pag_mount, tp,
+					    pag->pag_agno, 0);
+		if (error)
+			return error;
+	}

 	ASSERT(xfs_perag_resv(pag, XFS_AG_RESV_METADATA)->ar_reserved +
 	       xfs_perag_resv(pag, XFS_AG_RESV_RMAPBT)->ar_reserved <=
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index c5513e5..dc41801 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -594,39 +594,6 @@ xfs_check_summary_counts(
 		return -EFSCORRUPTED;
 	}

-	/*
-	 * Now the log is mounted, we know if it was an unclean shutdown or
-	 * not. If it was, with the first phase of recovery has completed, we
-	 * have consistent AG blocks on disk. We have not recovered EFIs yet,
-	 * but they are recovered transactionally in the second recovery phase
-	 * later.
-	 *
-	 * If the log was clean when we mounted, we can check the summary
-	 * counters.  If any of them are obviously incorrect, we can recompute
-	 * them from the AGF headers in the next step.
-	 */
-	if (XFS_LAST_UNMOUNT_WAS_CLEAN(mp) &&
-	    (mp->m_sb.sb_fdblocks > mp->m_sb.sb_dblocks ||
-	     !xfs_verify_icount(mp, mp->m_sb.sb_icount) ||
-	     mp->m_sb.sb_ifree > mp->m_sb.sb_icount))
-		xfs_fs_mark_sick(mp, XFS_SICK_FS_COUNTERS);
-
-	/*
-	 * We can safely re-initialise incore superblock counters from the
-	 * per-ag data. These may not be correct if the filesystem was not
-	 * cleanly unmounted, so we waited for recovery to finish before doing
-	 * this.
-	 *
-	 * If the filesystem was cleanly unmounted or the previous check did
-	 * not flag anything weird, then we can trust the values in the
-	 * superblock to be correct and we don't need to do anything here.
-	 * Otherwise, recalculate the summary counters.
-	 */
-	if ((!xfs_sb_version_haslazysbcount(&mp->m_sb) ||
-	     XFS_LAST_UNMOUNT_WAS_CLEAN(mp)) &&
-	    !xfs_fs_has_sickness(mp, XFS_SICK_FS_COUNTERS))
-		return 0;
-
 	return xfs_initialize_perag_data(mp, mp->m_sb.sb_agcount);
 }

--
2.7.4

[PATCH v2 2/2] xfs: avoid f_bfree overflow

[Zheng Bin]

If fdblocks < mp->m_alloc_set_aside, statp->f_bfree will overflow.
When we df -h /mnt(xfs mount point), will show this:
Filesystem      Size  Used Avail Use% Mounted on
/dev/loop0       17M  -64Z  -32K 100% /mnt

We can construct an img like this:

dd if=/dev/zero of=xfs.img bs=1M count=20
mkfs.xfs -d agcount=1 xfs.img
xfs_db -x xfs.img
sb 0
write fdblocks 0
agf 0
write freeblks 0
write longest 0
quit

Make sure statp->f_bfree does not underflow.
PS: add fdblocks check in mount.

Signed-off-by: Zheng Bin <zhengbin13@huawei.com>
---
 fs/xfs/xfs_mount.c | 6 ++++++
 fs/xfs/xfs_super.c | 3 ++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index dc41801..a223af4 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -816,6 +816,12 @@ xfs_mountfs(
 	if (error)
 		goto out_log_dealloc;

+	if (sbp->sb_fdblocks < mp->m_alloc_set_aside) {
+		xfs_alert(mp, "Corruption detected. Please run xfs_repair.");
+		error = -EFSCORRUPTED;
+		goto out_log_dealloc;
+	}
+
 	/*
 	 * Get and sanity-check the root inode.
 	 * Save the pointer to it in the mount structure.
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 2094386..9dcf772 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -755,7 +755,8 @@ xfs_fs_statfs(
 	statp->f_blocks = sbp->sb_dblocks - lsize;
 	spin_unlock(&mp->m_sb_lock);

-	statp->f_bfree = fdblocks - mp->m_alloc_set_aside;
+	/* make sure statp->f_bfree does not underflow */
+	statp->f_bfree = max_t(int64_t, fdblocks - mp->m_alloc_set_aside, 0);
 	statp->f_bavail = statp->f_bfree;

 	fakeinos = XFS_FSB_TO_INO(mp, statp->f_bfree);
--
2.7.4

Re: [PATCH v2 1/2] xfs: always init fdblocks in mount

[Christoph Hellwig]

So if we can't trust the sb counters on your fuzzed file system,
how can we trust the AG counters?

Also if we decide that we always want to rebuild and thus always read
in the pagf all the pagf_init checks can go away..