xfs: don't verify checksum on non-V5 superblocks

The current test in xfs_sb_read_verify() will attempt to validate an sb checksum if sb_crc is non-zero, even if the superblock is not marked as being version 5.

This runs the risk of picking up random garbage in sb_crc for non-V5 superblocks; such garbage is known to exist in the wild due to prior bugs. This will cause verification to fail for otherwise non-fatal reasons.

I'm not sure of the point of trying to validate a non-V5 superblock; is there one? Shouldn't this || be an &&? (Can sb_crc validly be 0 for a V5 SB?)

Signed-off-by Eric Sandeen <[email protected]>

---

fs/xfs/xfs_mount.c +1 -1

index 2b0ba35..5ca299b 100644
@@ -764,7 +764,7 @@ xfs_sb_read_verify(
 	 */
 	if (dsb->sb_magicnum == cpu_to_be32(XFS_SB_MAGIC) &&
 	    (((be16_to_cpu(dsb->sb_versionnum) & XFS_SB_VERSION_NUMBITS) ==
-						XFS_SB_VERSION_5) ||
+						XFS_SB_VERSION_5) &&
 	     dsb->sb_crc != 0)) {
 
 		if (!xfs_verify_cksum(bp->b_addr, be16_to_cpu(dsb->sb_sectsize),

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

The current test in xfs_sb_read_verify() will attempt to validate an sb checksum if sb_crc is non-zero, even if the superblock is not marked as being version 5.

This runs the risk of picking up random garbage in sb_crc for non-V5 superblocks; such garbage is known to exist in the wild due to prior bugs. This will cause verification to fail for otherwise non-fatal reasons.

I'm not sure of the point of trying to validate a non-V5 superblock; is there one? Shouldn't this || be an &&? (Can sb_crc validly be 0 for a V5 SB?)

Quoted 11 lines · Eric Sandeen

The current test in xfs_sb_read_verify() will attempt to validate an sb checksum if sb_crc is non-zero, even if the superblock is not marked as being version 5.

This runs the risk of picking up random garbage in sb_crc for non-V5 superblocks; such garbage is known to exist in the wild due to prior bugs. This will cause verification to fail for otherwise non-fatal reasons.

I'm not sure of the point of trying to validate a non-V5 superblock; is there one? Shouldn't this || be an &&? (Can sb_crc validly be 0 for a V5 SB?)

I don't think so.

As I mentioned on the call, the reason for this check is that if we have a CRC set and a non-v5 superblock version, we may have a corrupt superblock with bit errors in it. In this case, we check the CRC to determine if the superblock is intact. If the CRC validates, then it means that we wrote a bad superblock to disk (i.e. a code bug). If it doesn't validate, then the superblock is in a corrupt state because all fields not understood by the v4 superblock should be zero.

That's why if the checksum fails we are returning EFSCORRUPTED.

The problem we see here is not the validation of the primary superblock - it's the secondary superblocks that have been written by growfs that are the problem. We already know that we are verifying a secondary superblock by the "check_inprogress" parameter. Hence if we get this problem on a secondary superblock we can verify it against the primary superblock via the struct xfs_mount (i.e. mp->m_sb.sb_versionnum) and determine whether we do indeed have a v4 or v5 superblock and hence determine whether we should error out or just warn about it.

Cheers,

Dave.

Today, if xfs_sb_read_verify encounters a v4 superblock with junk past v4 fields which includes data in sb_crc, it will be treated as a failing checksum and significant corruption.

There are known prior bugs which leave junk at the end of the superblock; we don't need to actually fail the verification in this case if other checks pan out ok.

So if this is a secondary superblock, and the primary superblock is not V5, don't treat this as a serious checksum failure.

We should probably check the garbage condition as we do in xfs_repair, and possibly warn about it or self-heal, but that's a different scope of work.

Signed-off-by Eric Sandeen <[email protected]>

---

Today, if xfs_sb_read_verify encounters a v4 superblock with junk past v4 fields which includes data in sb_crc, it will be treated as a failing checksum and a significant corruption.

There are known prior bugs which leave junk at the end of the V4 superblock; we don't need to actually fail the verification in this case if other checks pan out ok.

So if this is a secondary superblock, and the primary superblock doesn't indicate that this is a V5 filesystem, don't treat this as an actual checksum failure.

We should probably check the garbage condition as we do in xfs_repair, and possibly warn about it or self-heal, but that's a different scope of work.

Stable folks: This can go back to v3.10, which is what introduced the sb CRC checking that is tripped up by old, stale, incorrect V4 superblocks w/ unzeroed bits.

Cc [email protected]

Signed-off-by Eric Sandeen<[email protected]>

---

V2: Comment changes: More! (No code changes)

fs/xfs/xfs_mount.c +5 -1

index 2b0ba35..b2deab1 100644
@@ -749,6 +749,11 @@ xfs_sb_verify(
   * single bit error could clear the feature bit and unused parts of the
   * superblock are supposed to be zero. Hence a non-null crc field indicates that
   * we've potentially lost a feature bit and we should check it anyway.
+ *
+ * However, past bugs (i.e. in growfs) left non-zeroed regions beyond the
+ * last field in V4 secondary superblocks.  So for secondary superblocks,
+ * we are more forgiving, and ignore CRC failures if the primary doesn't
+ * indicate that the fs version is V5.
   */
  static void
  xfs_sb_read_verify(

@@ -769,8 +774,12 @@ xfs_sb_read_verify(
 
  		if (!xfs_verify_cksum(bp->b_addr, be16_to_cpu(dsb->sb_sectsize),
  				      offsetof(struct xfs_sb, sb_crc))) {
-			error = EFSCORRUPTED;

Quoted 45 lines · Eric Sandeen

Today, if xfs_sb_read_verify encounters a v4 superblock with junk past v4 fields which includes data in sb_crc, it will be treated as a failing checksum and a significant corruption.

There are known prior bugs which leave junk at the end of the V4 superblock; we don't need to actually fail the verification in this case if other checks pan out ok.

So if this is a secondary superblock, and the primary superblock doesn't indicate that this is a V5 filesystem, don't treat this as an actual checksum failure.

We should probably check the garbage condition as we do in xfs_repair, and possibly warn about it or self-heal, but that's a different scope of work.

Stable folks: This can go back to v3.10, which is what introduced the sb CRC checking that is tripped up by old, stale, incorrect V4 superblocks w/ unzeroed bits.

Cc [email protected]

Signed-off-by Eric Sandeen<[email protected]>

---

V2: Comment changes: More! (No code changes)

fs/xfs/xfs_mount.c +5 -0

index 2b0ba35..b2deab1 100644
@@ -749,6 +749,11 @@ xfs_sb_verify(
   * single bit error could clear the feature bit and unused parts of the
   * superblock are supposed to be zero. Hence a non-null crc field indicates that
   * we've potentially lost a feature bit and we should check it anyway.
+ *
+ * However, past bugs (i.e. in growfs) left non-zeroed regions beyond the
+ * last field in V4 secondary superblocks.  So for secondary superblocks,
+ * we are more forgiving, and ignore CRC failures if the primary doesn't
+ * indicate that the fs version is V5.
   */
  static void
  xfs_sb_read_verify(

@@ -769,8 +774,12 @@ xfs_sb_read_verify(
 

fs/xfs/xfs_mount.c xfs_sb_read_verify( · line 785

Quoted 13 lines

fs/xfs/xfs_mount.c +6 -2

          if (!xfs_verify_cksum(bp->b_addr, be16_to_cpu(dsb->sb_sectsize),
                        offsetof(struct xfs_sb, sb_crc))) {
-            error = EFSCORRUPTED;
-            goto out_error;
+            /* Only fail bad secondaries on a known V5 filesystem */
+            if (bp->b_bn != XFS_SB_DADDR&&
+                xfs_sb_version_hascrc(&mp->m_sb)) {
+                error = EFSCORRUPTED;
+                goto out_error;
+            }
          }
      }
      error = xfs_sb_verify(bp, true);

This moved to fs/xfs/xfs_sb.c in TOT, but the patch looks good to me.

Reviewed-by Mark Tinguely <[email protected]>

Quoted 46 lines · Mark Tinguely

Quoted 45 lines · Eric Sandeen

Today, if xfs_sb_read_verify encounters a v4 superblock with junk past v4 fields which includes data in sb_crc, it will be treated as a failing checksum and a significant corruption.

There are known prior bugs which leave junk at the end of the V4 superblock; we don't need to actually fail the verification in this case if other checks pan out ok.

So if this is a secondary superblock, and the primary superblock doesn't indicate that this is a V5 filesystem, don't treat this as an actual checksum failure.

We should probably check the garbage condition as we do in xfs_repair, and possibly warn about it or self-heal, but that's a different scope of work.

Stable folks: This can go back to v3.10, which is what introduced the sb CRC checking that is tripped up by old, stale, incorrect V4 superblocks w/ unzeroed bits.

Cc [email protected]

Signed-off-by Eric Sandeen<[email protected]>

---

V2: Comment changes: More! (No code changes)

fs/xfs/xfs_mount.c +5 -0

index 2b0ba35..b2deab1 100644
@@ -749,6 +749,11 @@ xfs_sb_verify(
    * single bit error could clear the feature bit and unused parts of the
    * superblock are supposed to be zero. Hence a non-null crc field indicates that
    * we've potentially lost a feature bit and we should check it anyway.
+ *
+ * However, past bugs (i.e. in growfs) left non-zeroed regions beyond the
+ * last field in V4 secondary superblocks.  So for secondary superblocks,
+ * we are more forgiving, and ignore CRC failures if the primary doesn't
+ * indicate that the fs version is V5.
    */
   static void
   xfs_sb_read_verify(

@@ -769,8 +774,12 @@ xfs_sb_read_verify(
 

Quoted 15 lines

fs/xfs/xfs_mount.c xfs_sb_read_verify( · line 785

Quoted 13 lines

fs/xfs/xfs_mount.c +6 -2

           if (!xfs_verify_cksum(bp->b_addr, be16_to_cpu(dsb->sb_sectsize),
                         offsetof(struct xfs_sb, sb_crc))) {
-            error = EFSCORRUPTED;
-            goto out_error;
+            /* Only fail bad secondaries on a known V5 filesystem */
+            if (bp->b_bn != XFS_SB_DADDR&&
+                xfs_sb_version_hascrc(&mp->m_sb)) {
+                error = EFSCORRUPTED;
+                goto out_error;
+            }
           }
       }
       error = xfs_sb_verify(bp, true);

This moved to fs/xfs/xfs_sb.c in TOT, but the patch looks good to me.

Whoops, sorry. Thanks for the review. Want a resend?

(Any idea why your mail client eats spaces? "if (bp->b_bn != XFS_SB_DADDR&&" isn't in the original patch...)

Today, if xfs_sb_read_verify encounters a v4 superblock with junk past v4 fields which includes data in sb_crc, it will be treated as a failing checksum and a significant corruption.

There are known prior bugs which leave junk at the end of the V4 superblock; we don't need to actually fail the verification in this case if other checks pan out ok.

So if this is a secondary superblock, and the primary superblock doesn't indicate that this is a V5 filesystem, don't treat this as an actual checksum failure.

We should probably check the garbage condition as we do in xfs_repair, and possibly warn about it or self-heal, but that's a different scope of work.

Stable folks: This can go back to v3.10, which is what introduced the sb CRC checking that is tripped up by old, stale, incorrect V4 superblocks w/ unzeroed bits.

Cc [email protected]

Signed-off-by Eric Sandeen <[email protected]>

Today, if xfs_sb_read_verify encounters a v4 superblock with junk past v4 fields which includes data in sb_crc, it will be treated as a failing checksum and significant corruption.

There are known prior bugs which leave junk at the end of the superblock; we don't need to actually fail the verification in this case if other checks pan out ok.

So if this is a secondary superblock, and the primary superblock is not V5, don't treat this as a serious checksum failure.

We should probably check the garbage condition as we do in xfs_repair, and possibly warn about it or self-heal, but that's a different scope of work.

Signed-off-by Eric Sandeen <[email protected]>