All of lore.kernel.org
 help / color / mirror / Atom feed
* [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, 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

* 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

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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.