* [PATCH 1/4] xfs: refactor superblock verifiers
2018-07-31 3:55 [PATCH v2 0/4] xfs-4.19: superblock verifier cleanups Darrick J. Wong
@ 2018-07-31 3:55 ` Darrick J. Wong
2018-07-31 3:55 ` [PATCH 2/4] libxfs: add more bounds checking to sb sanity checks Darrick J. Wong
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Darrick J. Wong @ 2018-07-31 3:55 UTC (permalink / raw)
To: darrick.wong; +Cc: linux-xfs, Eric Sandeen
From: Darrick J. Wong <darrick.wong@oracle.com>
Split the superblock verifier into the common checks, the read-time
checks, and the write-time check functions. No functional changes, but
we're setting up to add more write-only checks.
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Eric Sandeen <sandeen@redhat.com>
---
fs/xfs/libxfs/xfs_sb.c | 205 ++++++++++++++++++++++++++----------------------
1 file changed, 111 insertions(+), 94 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
index b3ad15956366..f3835e923893 100644
--- a/fs/xfs/libxfs/xfs_sb.c
+++ b/fs/xfs/libxfs/xfs_sb.c
@@ -96,80 +96,94 @@ xfs_perag_put(
trace_xfs_perag_put(pag->pag_mount, pag->pag_agno, ref, _RET_IP_);
}
-/*
- * Check the validity of the SB found.
- */
+/* Check all the superblock fields we care about when reading one in. */
STATIC int
-xfs_mount_validate_sb(
- xfs_mount_t *mp,
- xfs_sb_t *sbp,
- bool check_inprogress,
- bool check_version)
+xfs_validate_sb_read(
+ struct xfs_mount *mp,
+ struct xfs_sb *sbp)
{
- uint32_t agcount = 0;
- uint32_t rem;
-
- if (sbp->sb_magicnum != XFS_SB_MAGIC) {
- xfs_warn(mp, "bad magic number");
- return -EWRONGFS;
- }
-
-
- if (!xfs_sb_good_version(sbp)) {
- xfs_warn(mp, "bad version");
- return -EWRONGFS;
- }
+ if (XFS_SB_VERSION_NUM(sbp) != XFS_SB_VERSION_5)
+ return 0;
/*
- * Version 5 superblock feature mask validation. Reject combinations the
- * kernel cannot support up front before checking anything else. For
- * write validation, we don't need to check feature masks.
+ * Version 5 superblock feature mask validation. Reject combinations
+ * the kernel cannot support up front before checking anything else.
*/
- if (check_version && XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5) {
- if (xfs_sb_has_compat_feature(sbp,
- XFS_SB_FEAT_COMPAT_UNKNOWN)) {
- xfs_warn(mp,
+ if (xfs_sb_has_compat_feature(sbp, XFS_SB_FEAT_COMPAT_UNKNOWN)) {
+ xfs_warn(mp,
"Superblock has unknown compatible features (0x%x) enabled.",
- (sbp->sb_features_compat &
- XFS_SB_FEAT_COMPAT_UNKNOWN));
- xfs_warn(mp,
+ (sbp->sb_features_compat & XFS_SB_FEAT_COMPAT_UNKNOWN));
+ xfs_warn(mp,
"Using a more recent kernel is recommended.");
- }
+ }
- if (xfs_sb_has_ro_compat_feature(sbp,
- XFS_SB_FEAT_RO_COMPAT_UNKNOWN)) {
- xfs_alert(mp,
+ if (xfs_sb_has_ro_compat_feature(sbp, XFS_SB_FEAT_RO_COMPAT_UNKNOWN)) {
+ xfs_alert(mp,
"Superblock has unknown read-only compatible features (0x%x) enabled.",
- (sbp->sb_features_ro_compat &
- XFS_SB_FEAT_RO_COMPAT_UNKNOWN));
- if (!(mp->m_flags & XFS_MOUNT_RDONLY)) {
- xfs_warn(mp,
+ (sbp->sb_features_ro_compat &
+ XFS_SB_FEAT_RO_COMPAT_UNKNOWN));
+ if (!(mp->m_flags & XFS_MOUNT_RDONLY)) {
+ xfs_warn(mp,
"Attempted to mount read-only compatible filesystem read-write.");
- xfs_warn(mp,
+ xfs_warn(mp,
"Filesystem can only be safely mounted read only.");
- return -EINVAL;
- }
- }
- if (xfs_sb_has_incompat_feature(sbp,
- XFS_SB_FEAT_INCOMPAT_UNKNOWN)) {
- xfs_warn(mp,
-"Superblock has unknown incompatible features (0x%x) enabled.",
- (sbp->sb_features_incompat &
- XFS_SB_FEAT_INCOMPAT_UNKNOWN));
- xfs_warn(mp,
-"Filesystem can not be safely mounted by this kernel.");
return -EINVAL;
}
- } else if (xfs_sb_version_hascrc(sbp)) {
- /*
- * We can't read verify the sb LSN because the read verifier is
- * called before the log is allocated and processed. We know the
- * log is set up before write verifier (!check_version) calls,
- * so just check it here.
- */
- if (!xfs_log_check_lsn(mp, sbp->sb_lsn))
- return -EFSCORRUPTED;
+ }
+ if (xfs_sb_has_incompat_feature(sbp, XFS_SB_FEAT_INCOMPAT_UNKNOWN)) {
+ xfs_warn(mp,
+"Superblock has unknown incompatible features (0x%x) enabled.",
+ (sbp->sb_features_incompat &
+ XFS_SB_FEAT_INCOMPAT_UNKNOWN));
+ xfs_warn(mp,
+"Filesystem cannot be safely mounted by this kernel.");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+/* Check all the superblock fields we care about when writing one out. */
+STATIC int
+xfs_validate_sb_write(
+ struct xfs_mount *mp,
+ struct xfs_sb *sbp)
+{
+ if (XFS_SB_VERSION_NUM(sbp) != XFS_SB_VERSION_5)
+ return 0;
+
+ /* XXX: For write validation, we don't need to check feature masks?? */
+
+ /*
+ * We can't read verify the sb LSN because the read verifier is called
+ * before the log is allocated and processed. We know the log is set up
+ * before write verifier calls, so check it here.
+ */
+ if (!xfs_log_check_lsn(mp, sbp->sb_lsn))
+ return -EFSCORRUPTED;
+
+ return 0;
+}
+
+/* Check the validity of the SB. */
+STATIC int
+xfs_validate_sb_common(
+ struct xfs_mount *mp,
+ struct xfs_buf *bp,
+ struct xfs_sb *sbp)
+{
+ uint32_t agcount = 0;
+ uint32_t rem;
+
+ if (sbp->sb_magicnum != XFS_SB_MAGIC) {
+ xfs_warn(mp, "bad magic number");
+ return -EWRONGFS;
+ }
+
+ if (!xfs_sb_good_version(sbp)) {
+ xfs_warn(mp, "bad version");
+ return -EWRONGFS;
}
if (xfs_sb_version_has_pquotino(sbp)) {
@@ -321,7 +335,12 @@ xfs_mount_validate_sb(
return -EFBIG;
}
- if (check_inprogress && sbp->sb_inprogress) {
+ /*
+ * Don't touch the filesystem if a user tool thinks it owns the primary
+ * superblock. mkfs doesn't clear the flag from secondary supers, so
+ * we don't check them at all.
+ */
+ if (XFS_BUF_ADDR(bp) == XFS_SB_DADDR && sbp->sb_inprogress) {
xfs_warn(mp, "Offline file system operation in progress!");
return -EFSCORRUPTED;
}
@@ -596,29 +615,6 @@ xfs_sb_to_disk(
}
}
-static int
-xfs_sb_verify(
- struct xfs_buf *bp,
- bool check_version)
-{
- struct xfs_mount *mp = bp->b_target->bt_mount;
- struct xfs_sb sb;
-
- /*
- * Use call variant which doesn't convert quota flags from disk
- * format, because xfs_mount_validate_sb checks the on-disk flags.
- */
- __xfs_sb_from_disk(&sb, XFS_BUF_TO_SBP(bp), false);
-
- /*
- * Only check the in progress field for the primary superblock as
- * mkfs.xfs doesn't clear it from secondary superblocks.
- */
- return xfs_mount_validate_sb(mp, &sb,
- bp->b_maps[0].bm_bn == XFS_SB_DADDR,
- check_version);
-}
-
/*
* If the superblock has the CRC feature bit set or the CRC field is non-null,
* check that the CRC is valid. We check the CRC field is non-null because a
@@ -633,11 +629,12 @@ xfs_sb_verify(
*/
static void
xfs_sb_read_verify(
- struct xfs_buf *bp)
+ struct xfs_buf *bp)
{
- struct xfs_mount *mp = bp->b_target->bt_mount;
- struct xfs_dsb *dsb = XFS_BUF_TO_SBP(bp);
- int error;
+ struct xfs_sb sb;
+ struct xfs_mount *mp = bp->b_target->bt_mount;
+ struct xfs_dsb *dsb = XFS_BUF_TO_SBP(bp);
+ int error;
/*
* open code the version check to avoid needing to convert the entire
@@ -657,7 +654,16 @@ xfs_sb_read_verify(
}
}
}
- error = xfs_sb_verify(bp, true);
+
+ /*
+ * Check all the superblock fields. Don't byteswap the xquota flags
+ * because _verify_common checks the on-disk values.
+ */
+ __xfs_sb_from_disk(&sb, XFS_BUF_TO_SBP(bp), false);
+ error = xfs_validate_sb_common(mp, bp, &sb);
+ if (error)
+ goto out_error;
+ error = xfs_validate_sb_read(mp, &sb);
out_error:
if (error == -EFSCORRUPTED || error == -EFSBADCRC)
@@ -691,15 +697,22 @@ static void
xfs_sb_write_verify(
struct xfs_buf *bp)
{
+ struct xfs_sb sb;
struct xfs_mount *mp = bp->b_target->bt_mount;
struct xfs_buf_log_item *bip = bp->b_log_item;
int error;
- error = xfs_sb_verify(bp, false);
- if (error) {
- xfs_verifier_error(bp, error, __this_address);
- return;
- }
+ /*
+ * Check all the superblock fields. Don't byteswap the xquota flags
+ * because _verify_common checks the on-disk values.
+ */
+ __xfs_sb_from_disk(&sb, XFS_BUF_TO_SBP(bp), false);
+ error = xfs_validate_sb_common(mp, bp, &sb);
+ if (error)
+ goto out_error;
+ error = xfs_validate_sb_write(mp, &sb);
+ if (error)
+ goto out_error;
if (!xfs_sb_version_hascrc(&mp->m_sb))
return;
@@ -708,6 +721,10 @@ xfs_sb_write_verify(
XFS_BUF_TO_SBP(bp)->sb_lsn = cpu_to_be64(bip->bli_item.li_lsn);
xfs_buf_update_cksum(bp, XFS_SB_CRC_OFF);
+ return;
+
+out_error:
+ xfs_verifier_error(bp, error, __this_address);
}
const struct xfs_buf_ops xfs_sb_buf_ops = {
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH 3/4] xfs: verify icount in superblock write
2018-07-31 3:55 [PATCH v2 0/4] xfs-4.19: superblock verifier cleanups Darrick J. Wong
2018-07-31 3:55 ` [PATCH 1/4] xfs: refactor superblock verifiers Darrick J. Wong
2018-07-31 3:55 ` [PATCH 2/4] libxfs: add more bounds checking to sb sanity checks Darrick J. Wong
@ 2018-07-31 3:55 ` Darrick J. Wong
2018-07-31 3:57 ` Eric Sandeen
2018-07-31 3:56 ` [PATCH 4/4] xfs: check for unknown v5 feature bits in superblock write verifier Darrick J. Wong
3 siblings, 1 reply; 7+ messages in thread
From: Darrick J. Wong @ 2018-07-31 3:55 UTC (permalink / raw)
To: darrick.wong; +Cc: linux-xfs, Bill O'Donnell
From: Darrick J. Wong <darrick.wong@oracle.com>
Add a helper predicate to check the inode count for sanity, then use it
in the superblock write verifier to inspect sb_icount.
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Bill O'Donnell <billodo@redhat.com>
---
fs/xfs/libxfs/xfs_sb.c | 3 ++-
fs/xfs/libxfs/xfs_types.c | 34 ++++++++++++++++++++++++++++++++++
fs/xfs/libxfs/xfs_types.h | 1 +
3 files changed, 37 insertions(+), 1 deletion(-)
diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
index 3d29f4a5242f..05e7ed1b8022 100644
--- a/fs/xfs/libxfs/xfs_sb.c
+++ b/fs/xfs/libxfs/xfs_sb.c
@@ -154,9 +154,10 @@ xfs_validate_sb_write(
* Carry out additional sb summary counter sanity checks when we write
* the superblock. We skip this in the read validator because there
* could be newer superblocks in the log and if the values are garbage
- * we'll recalculate them at the end of log mount.
+ * even after replay we'll recalculate them at the end of log mount.
*/
if (sbp->sb_fdblocks > sbp->sb_dblocks ||
+ !xfs_verify_icount(mp, sbp->sb_icount) ||
sbp->sb_ifree > sbp->sb_icount) {
xfs_warn(mp, "SB summary counter sanity check failed");
return -EFSCORRUPTED;
diff --git a/fs/xfs/libxfs/xfs_types.c b/fs/xfs/libxfs/xfs_types.c
index 2e2a243cef2e..33a5ca346baf 100644
--- a/fs/xfs/libxfs/xfs_types.c
+++ b/fs/xfs/libxfs/xfs_types.c
@@ -171,3 +171,37 @@ xfs_verify_rtbno(
{
return rtbno < mp->m_sb.sb_rblocks;
}
+
+/* Calculate the range of valid icount values. */
+static void
+xfs_icount_range(
+ struct xfs_mount *mp,
+ unsigned long long *min,
+ unsigned long long *max)
+{
+ unsigned long long nr_inos = 0;
+ xfs_agnumber_t agno;
+
+ /* root, rtbitmap, rtsum all live in the first chunk */
+ *min = XFS_INODES_PER_CHUNK;
+
+ for (agno = 0; agno < mp->m_sb.sb_agcount; agno++) {
+ xfs_agino_t first, last;
+
+ xfs_agino_range(mp, agno, &first, &last);
+ nr_inos += last - first + 1;
+ }
+ *max = nr_inos;
+}
+
+/* Sanity-checking of inode counts. */
+bool
+xfs_verify_icount(
+ struct xfs_mount *mp,
+ unsigned long long icount)
+{
+ unsigned long long min, max;
+
+ xfs_icount_range(mp, &min, &max);
+ return icount >= min && icount <= max;
+}
diff --git a/fs/xfs/libxfs/xfs_types.h b/fs/xfs/libxfs/xfs_types.h
index 4055d62f690c..b9e6c89284c3 100644
--- a/fs/xfs/libxfs/xfs_types.h
+++ b/fs/xfs/libxfs/xfs_types.h
@@ -165,5 +165,6 @@ bool xfs_verify_ino(struct xfs_mount *mp, xfs_ino_t ino);
bool xfs_internal_inum(struct xfs_mount *mp, xfs_ino_t ino);
bool xfs_verify_dir_ino(struct xfs_mount *mp, xfs_ino_t ino);
bool xfs_verify_rtbno(struct xfs_mount *mp, xfs_rtblock_t rtbno);
+bool xfs_verify_icount(struct xfs_mount *mp, unsigned long long icount);
#endif /* __XFS_TYPES_H__ */
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH 4/4] xfs: check for unknown v5 feature bits in superblock write verifier
2018-07-31 3:55 [PATCH v2 0/4] xfs-4.19: superblock verifier cleanups Darrick J. Wong
` (2 preceding siblings ...)
2018-07-31 3:55 ` [PATCH 3/4] xfs: verify icount in superblock write Darrick J. Wong
@ 2018-07-31 3:56 ` Darrick J. Wong
2018-07-31 4:07 ` Eric Sandeen
3 siblings, 1 reply; 7+ messages in thread
From: Darrick J. Wong @ 2018-07-31 3:56 UTC (permalink / raw)
To: darrick.wong; +Cc: linux-xfs
From: Darrick J. Wong <darrick.wong@oracle.com>
Make sure we never try to write the superblock with unknown feature bits
set. We checked those at mount time, so if they're set now then memory
is corrupt.
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
fs/xfs/libxfs/xfs_sb.c | 35 ++++++++++++++++++++++++++++++++++-
1 file changed, 34 insertions(+), 1 deletion(-)
diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
index 05e7ed1b8022..ca1b3a7a9171 100644
--- a/fs/xfs/libxfs/xfs_sb.c
+++ b/fs/xfs/libxfs/xfs_sb.c
@@ -166,7 +166,40 @@ xfs_validate_sb_write(
if (XFS_SB_VERSION_NUM(sbp) != XFS_SB_VERSION_5)
return 0;
- /* XXX: For write validation, we don't need to check feature masks?? */
+ /*
+ * Version 5 superblock feature mask validation. Reject combinations
+ * the kernel cannot support since we checked for unsupported bits in
+ * the read verifier, which means that memory is corrupt.
+ */
+ if (xfs_sb_has_compat_feature(sbp, XFS_SB_FEAT_COMPAT_UNKNOWN)) {
+ xfs_warn(mp,
+"Corruption detected in superblock compatible features (0x%x)!",
+ (sbp->sb_features_compat & XFS_SB_FEAT_COMPAT_UNKNOWN));
+ return -EFSCORRUPTED;
+ }
+
+ if (xfs_sb_has_ro_compat_feature(sbp, XFS_SB_FEAT_RO_COMPAT_UNKNOWN)) {
+ xfs_alert(mp,
+"Corruption detected in superblock read-only compatible features (0x%x)!",
+ (sbp->sb_features_ro_compat &
+ XFS_SB_FEAT_RO_COMPAT_UNKNOWN));
+ return -EFSCORRUPTED;
+ }
+ if (xfs_sb_has_incompat_feature(sbp, XFS_SB_FEAT_INCOMPAT_UNKNOWN)) {
+ xfs_warn(mp,
+"Corruption detected in superblock incompatible features (0x%x)!",
+ (sbp->sb_features_incompat &
+ XFS_SB_FEAT_INCOMPAT_UNKNOWN));
+ return -EFSCORRUPTED;
+ }
+ if (xfs_sb_has_incompat_log_feature(sbp,
+ XFS_SB_FEAT_INCOMPAT_LOG_UNKNOWN)) {
+ xfs_warn(mp,
+"Corruption detected in superblock incompatible log features (0x%x)!",
+ (sbp->sb_features_log_incompat &
+ XFS_SB_FEAT_INCOMPAT_LOG_UNKNOWN));
+ return -EFSCORRUPTED;
+ }
/*
* We can't read verify the sb LSN because the read verifier is called
^ permalink raw reply related [flat|nested] 7+ messages in thread