* [PATCH, RFC] xfs: don't verify checksum on non-V5 superblocks @ 2013-08-15 18:19 Eric Sandeen 2013-08-15 19:45 ` Ben Myers 2013-08-15 21:00 ` Dave Chinner 0 siblings, 2 replies; 12+ messages in thread From: Eric Sandeen @ 2013-08-15 18:19 UTC (permalink / raw) To: 'linux-xfs@oss.sgi.com' 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 <sandeen@redhat.com> --- diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c index 2b0ba35..5ca299b 100644 --- a/fs/xfs/xfs_mount.c +++ b/fs/xfs/xfs_mount.c @@ -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 xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH, RFC] xfs: don't verify checksum on non-V5 superblocks 2013-08-15 18:19 [PATCH, RFC] xfs: don't verify checksum on non-V5 superblocks Eric Sandeen @ 2013-08-15 19:45 ` Ben Myers 2013-08-15 21:00 ` Dave Chinner 1 sibling, 0 replies; 12+ messages in thread From: Ben Myers @ 2013-08-15 19:45 UTC (permalink / raw) To: Eric Sandeen; +Cc: 'linux-xfs@oss.sgi.com' On Thu, Aug 15, 2013 at 01:19:15PM -0500, Eric Sandeen wrote: > 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 <sandeen@redhat.com> This looks good to me. Reviewed-by: Ben Myers <bpm@sgi.com> > --- > > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c > index 2b0ba35..5ca299b 100644 > --- a/fs/xfs/xfs_mount.c > +++ b/fs/xfs/xfs_mount.c > @@ -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 > xfs@oss.sgi.com > http://oss.sgi.com/mailman/listinfo/xfs _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH, RFC] xfs: don't verify checksum on non-V5 superblocks 2013-08-15 18:19 [PATCH, RFC] xfs: don't verify checksum on non-V5 superblocks Eric Sandeen 2013-08-15 19:45 ` Ben Myers @ 2013-08-15 21:00 ` Dave Chinner 2013-08-15 21:15 ` Eric Sandeen 1 sibling, 1 reply; 12+ messages in thread From: Dave Chinner @ 2013-08-15 21:00 UTC (permalink / raw) To: Eric Sandeen; +Cc: 'linux-xfs@oss.sgi.com' On Thu, Aug 15, 2013 at 01:19:15PM -0500, Eric Sandeen wrote: > 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. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH, RFC] xfs: don't verify checksum on non-V5 superblocks 2013-08-15 21:00 ` Dave Chinner @ 2013-08-15 21:15 ` Eric Sandeen 2013-08-15 22:41 ` [PATCH, RFC] xfs: be more forgiving of a v4 secondary sb w/ junk in v5 fields Eric Sandeen 0 siblings, 1 reply; 12+ messages in thread From: Eric Sandeen @ 2013-08-15 21:15 UTC (permalink / raw) To: Dave Chinner; +Cc: 'linux-xfs@oss.sgi.com', Eric Sandeen On 8/15/13 4:00 PM, Dave Chinner wrote: > On Thu, Aug 15, 2013 at 01:19:15PM -0500, Eric Sandeen wrote: >> 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. Ok, let me resend a patch under the same subject, different implementation :) -Eric > Cheers, > > Dave. > _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH, RFC] xfs: be more forgiving of a v4 secondary sb w/ junk in v5 fields 2013-08-15 21:15 ` Eric Sandeen @ 2013-08-15 22:41 ` Eric Sandeen 2013-08-15 23:15 ` Dave Chinner ` (2 more replies) 0 siblings, 3 replies; 12+ messages in thread From: Eric Sandeen @ 2013-08-15 22:41 UTC (permalink / raw) To: Eric Sandeen; +Cc: 'linux-xfs@oss.sgi.com' 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 <sandeen@redhat.com> --- diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c index 2b0ba35..5ce25ae 100644 --- a/fs/xfs/xfs_mount.c +++ b/fs/xfs/xfs_mount.c @@ -769,8 +769,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; - goto out_error; + /* Only fail here for a real 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); _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH, RFC] xfs: be more forgiving of a v4 secondary sb w/ junk in v5 fields 2013-08-15 22:41 ` [PATCH, RFC] xfs: be more forgiving of a v4 secondary sb w/ junk in v5 fields Eric Sandeen @ 2013-08-15 23:15 ` Dave Chinner 2013-09-09 20:33 ` [PATCH V2] " Eric Sandeen 2013-10-17 20:17 ` [PATCH, RFC] " Ben Myers 2 siblings, 0 replies; 12+ messages in thread From: Dave Chinner @ 2013-08-15 23:15 UTC (permalink / raw) To: Eric Sandeen; +Cc: 'linux-xfs@oss.sgi.com', Eric Sandeen On Thu, Aug 15, 2013 at 05:41:49PM -0500, Eric Sandeen wrote: > 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 <sandeen@redhat.com> > --- Fine by me. Reviewed-by: Dave Chinner <dchinner@redhat.com> Cheers, Dave. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH V2] xfs: be more forgiving of a v4 secondary sb w/ junk in v5 fields 2013-08-15 22:41 ` [PATCH, RFC] xfs: be more forgiving of a v4 secondary sb w/ junk in v5 fields Eric Sandeen 2013-08-15 23:15 ` Dave Chinner @ 2013-09-09 20:33 ` Eric Sandeen 2013-09-09 21:08 ` Mark Tinguely 2013-10-31 15:51 ` Ben Myers 2013-10-17 20:17 ` [PATCH, RFC] " Ben Myers 2 siblings, 2 replies; 12+ messages in thread From: Eric Sandeen @ 2013-09-09 20:33 UTC (permalink / raw) To: Eric Sandeen; +Cc: 'linux-xfs@oss.sgi.com' 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: stable@vger.kernel.org Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- V2: Comment changes: More! (No code changes) diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c index 2b0ba35..b2deab1 100644 --- a/fs/xfs/xfs_mount.c +++ b/fs/xfs/xfs_mount.c @@ -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; - 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); _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH V2] xfs: be more forgiving of a v4 secondary sb w/ junk in v5 fields 2013-09-09 20:33 ` [PATCH V2] " Eric Sandeen @ 2013-09-09 21:08 ` Mark Tinguely 2013-09-09 21:10 ` Eric Sandeen 2013-10-31 15:51 ` Ben Myers 1 sibling, 1 reply; 12+ messages in thread From: Mark Tinguely @ 2013-09-09 21:08 UTC (permalink / raw) To: Eric Sandeen; +Cc: 'linux-xfs@oss.sgi.com', Eric Sandeen On 09/09/13 15:33, Eric Sandeen wrote: > 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: stable@vger.kernel.org > Signed-off-by: Eric Sandeen<sandeen@redhat.com> > --- > > V2: Comment changes: More! (No code changes) > > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c > index 2b0ba35..b2deab1 100644 > --- a/fs/xfs/xfs_mount.c > +++ b/fs/xfs/xfs_mount.c > @@ -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; > - 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 <tinguely@sgi.com> _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V2] xfs: be more forgiving of a v4 secondary sb w/ junk in v5 fields 2013-09-09 21:08 ` Mark Tinguely @ 2013-09-09 21:10 ` Eric Sandeen 2013-09-09 21:16 ` Mark Tinguely 0 siblings, 1 reply; 12+ messages in thread From: Eric Sandeen @ 2013-09-09 21:10 UTC (permalink / raw) To: Mark Tinguely; +Cc: 'linux-xfs@oss.sgi.com', Eric Sandeen On 9/9/13 4:08 PM, Mark Tinguely wrote: > On 09/09/13 15:33, Eric Sandeen wrote: >> 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: stable@vger.kernel.org >> Signed-off-by: Eric Sandeen<sandeen@redhat.com> >> --- >> >> V2: Comment changes: More! (No code changes) >> >> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c >> index 2b0ba35..b2deab1 100644 >> --- a/fs/xfs/xfs_mount.c >> +++ b/fs/xfs/xfs_mount.c >> @@ -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; >> - 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...) > Reviewed-by: Mark Tinguely <tinguely@sgi.com> Thanks, -Eric _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V2] xfs: be more forgiving of a v4 secondary sb w/ junk in v5 fields 2013-09-09 21:10 ` Eric Sandeen @ 2013-09-09 21:16 ` Mark Tinguely 0 siblings, 0 replies; 12+ messages in thread From: Mark Tinguely @ 2013-09-09 21:16 UTC (permalink / raw) To: Eric Sandeen; +Cc: 'linux-xfs@oss.sgi.com', Eric Sandeen On 09/09/13 16:10, Eric Sandeen wrote: > On 9/9/13 4:08 PM, Mark Tinguely wrote: >> On 09/09/13 15:33, Eric Sandeen wrote: >>> 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: stable@vger.kernel.org >>> Signed-off-by: Eric Sandeen<sandeen@redhat.com> >>> --- >>> >>> V2: Comment changes: More! (No code changes) >>> >>> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c >>> index 2b0ba35..b2deab1 100644 >>> --- a/fs/xfs/xfs_mount.c >>> +++ b/fs/xfs/xfs_mount.c >>> @@ -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; >>> - 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? Since Ben will do all the work, not necessary. ;) > > (Any idea why your mail client eats spaces? "if (bp->b_bn != XFS_SB_DADDR&&" isn't > in the original patch...) Dave mentioned that too before, I will check into it. --Mark. _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V2] xfs: be more forgiving of a v4 secondary sb w/ junk in v5 fields 2013-09-09 20:33 ` [PATCH V2] " Eric Sandeen 2013-09-09 21:08 ` Mark Tinguely @ 2013-10-31 15:51 ` Ben Myers 1 sibling, 0 replies; 12+ messages in thread From: Ben Myers @ 2013-10-31 15:51 UTC (permalink / raw) To: Eric Sandeen; +Cc: 'linux-xfs@oss.sgi.com', Eric Sandeen Hey Eric, On Mon, Sep 09, 2013 at 03:33:29PM -0500, Eric Sandeen wrote: > 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: stable@vger.kernel.org > Signed-off-by: Eric Sandeen <sandeen@redhat.com> Applied this one. Thanks. -Ben _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH, RFC] xfs: be more forgiving of a v4 secondary sb w/ junk in v5 fields 2013-08-15 22:41 ` [PATCH, RFC] xfs: be more forgiving of a v4 secondary sb w/ junk in v5 fields Eric Sandeen 2013-08-15 23:15 ` Dave Chinner 2013-09-09 20:33 ` [PATCH V2] " Eric Sandeen @ 2013-10-17 20:17 ` Ben Myers 2 siblings, 0 replies; 12+ messages in thread From: Ben Myers @ 2013-10-17 20:17 UTC (permalink / raw) To: Eric Sandeen; +Cc: 'linux-xfs@oss.sgi.com', Eric Sandeen On Thu, Aug 15, 2013 at 05:41:49PM -0500, Eric Sandeen wrote: > 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 <sandeen@redhat.com> Eric... is the one you're talking about? _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2013-10-31 15:51 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-08-15 18:19 [PATCH, RFC] xfs: don't verify checksum on non-V5 superblocks Eric Sandeen 2013-08-15 19:45 ` Ben Myers 2013-08-15 21:00 ` Dave Chinner 2013-08-15 21:15 ` Eric Sandeen 2013-08-15 22:41 ` [PATCH, RFC] xfs: be more forgiving of a v4 secondary sb w/ junk in v5 fields Eric Sandeen 2013-08-15 23:15 ` Dave Chinner 2013-09-09 20:33 ` [PATCH V2] " Eric Sandeen 2013-09-09 21:08 ` Mark Tinguely 2013-09-09 21:10 ` Eric Sandeen 2013-09-09 21:16 ` Mark Tinguely 2013-10-31 15:51 ` Ben Myers 2013-10-17 20:17 ` [PATCH, RFC] " Ben Myers
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox