All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] xfs: check magic numbers in dir3 leaf verifier first
@ 2013-09-02 10:50 Dave Chinner
  2013-09-03  0:06 ` [PATCH v2] " Dave Chinner
  0 siblings, 1 reply; 5+ messages in thread
From: Dave Chinner @ 2013-09-02 10:50 UTC (permalink / raw)
  To: xfs

From: Dave Chinner <dchinner@redhat.com>

Calling xfs_dir3_leaf_hdr_from_disk() in a verifier before
validating the magic numbers in the buffer results in ASSERT
failures due to mismatching magic numbers when a corruption occurs.
Seeing as the verifier is supposed to catch the corruption and pass
it back to the caller, having the verifier assert fail on error
defeats the purpose of detecting the errors in the first place.

Check the magic numbers direct from the buffer before decoding the
header.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_dir2_leaf.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/fs/xfs/xfs_dir2_leaf.c b/fs/xfs/xfs_dir2_leaf.c
index 08984ee..fa46d4b 100644
--- a/fs/xfs/xfs_dir2_leaf.c
+++ b/fs/xfs/xfs_dir2_leaf.c
@@ -165,6 +165,7 @@ xfs_dir3_leaf_check_int(
 	    (char *)&ents[hdr->count] > (char *)xfs_dir2_leaf_bests_p(ltp))
 		return false;
 
+
 	/* Check hash value order, count stale entries.  */
 	for (i = stale = 0; i < hdr->count; i++) {
 		if (i + 1 < hdr->count) {
@@ -180,6 +181,11 @@ xfs_dir3_leaf_check_int(
 	return true;
 }
 
+/*
+ * We verify the magic numbers before decoding the leaf header so that on debug
+ * kernels we don't get assertion failures in xfs_dir3_leaf_hdr_from_disk() due
+ * to incorrect magic numbers.
+ */
 static bool
 xfs_dir3_leaf_verify(
 	struct xfs_buf		*bp,
@@ -191,24 +197,25 @@ xfs_dir3_leaf_verify(
 
 	ASSERT(magic == XFS_DIR2_LEAF1_MAGIC || magic == XFS_DIR2_LEAFN_MAGIC);
 
-	xfs_dir3_leaf_hdr_from_disk(&leafhdr, leaf);
 	if (xfs_sb_version_hascrc(&mp->m_sb)) {
 		struct xfs_dir3_leaf_hdr *leaf3 = bp->b_addr;
+		__uint16_t		magic3;
 
-		if ((magic == XFS_DIR2_LEAF1_MAGIC &&
-		     leafhdr.magic != XFS_DIR3_LEAF1_MAGIC) ||
-		    (magic == XFS_DIR2_LEAFN_MAGIC &&
-		     leafhdr.magic != XFS_DIR3_LEAFN_MAGIC))
-			return false;
+		magic3 = (magic == XFS_DIR2_LEAF1_MAGIC) ? XFS_DIR3_LEAF1_MAGIC
+							 : XFS_DIR3_LEAFN_MAGIC;
 
+		if (leaf3->info.hdr.magic != cpu_to_be16(magic3))
+			return false;
 		if (!uuid_equal(&leaf3->info.uuid, &mp->m_sb.sb_uuid))
 			return false;
 		if (be64_to_cpu(leaf3->info.blkno) != bp->b_bn)
 			return false;
 	} else {
-		if (leafhdr.magic != magic)
+		if (leaf->hdr.info.magic != cpu_to_be32(magic))
 			return false;
 	}
+
+	xfs_dir3_leaf_hdr_from_disk(&leafhdr, leaf);
 	return xfs_dir3_leaf_check_int(mp, &leafhdr, leaf);
 }
 
-- 
1.8.3.2

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply related	[flat|nested] 5+ messages in thread
* [PATCH v2] xfs: check magic numbers in dir3 leaf verifier first
  2013-09-02 10:50 [PATCH] xfs: check magic numbers in dir3 leaf verifier first Dave Chinner
@ 2013-09-03  0:06 ` Dave Chinner
  2013-09-03  7:33   ` Christoph Hellwig
                     ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Dave Chinner @ 2013-09-03  0:06 UTC (permalink / raw)
  To: xfs

xfs: check magic numbers in dir3 leaf verifier first

From: Dave Chinner <dchinner@redhat.com>

Calling xfs_dir3_leaf_hdr_from_disk() in a verifier before
validating the magic numbers in the buffer results in ASSERT
failures due to mismatching magic numbers when a corruption occurs.
Seeing as the verifier is supposed to catch the corruption and pass
it back to the caller, having the verifier assert fail on error
defeats the purpose of detecting the errors in the first place.

Check the magic numbers direct from the buffer before decoding the
header.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
v2: magic number size is 16 bits, not 32 bits. Was only wrong for v4
filesystem checking, xfstests/204 triggered failure.

 fs/xfs/xfs_dir2_leaf.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/fs/xfs/xfs_dir2_leaf.c b/fs/xfs/xfs_dir2_leaf.c
index 08984ee..fb57893 100644
--- a/fs/xfs/xfs_dir2_leaf.c
+++ b/fs/xfs/xfs_dir2_leaf.c
@@ -165,6 +165,7 @@ xfs_dir3_leaf_check_int(
 	    (char *)&ents[hdr->count] > (char *)xfs_dir2_leaf_bests_p(ltp))
 		return false;
 
+
 	/* Check hash value order, count stale entries.  */
 	for (i = stale = 0; i < hdr->count; i++) {
 		if (i + 1 < hdr->count) {
@@ -180,6 +181,11 @@ xfs_dir3_leaf_check_int(
 	return true;
 }
 
+/*
+ * We verify the magic numbers before decoding the leaf header so that on debug
+ * kernels we don't get assertion failures in xfs_dir3_leaf_hdr_from_disk() due
+ * to incorrect magic numbers.
+ */
 static bool
 xfs_dir3_leaf_verify(
 	struct xfs_buf		*bp,
@@ -191,24 +197,25 @@ xfs_dir3_leaf_verify(
 
 	ASSERT(magic == XFS_DIR2_LEAF1_MAGIC || magic == XFS_DIR2_LEAFN_MAGIC);
 
-	xfs_dir3_leaf_hdr_from_disk(&leafhdr, leaf);
 	if (xfs_sb_version_hascrc(&mp->m_sb)) {
 		struct xfs_dir3_leaf_hdr *leaf3 = bp->b_addr;
+		__uint16_t		magic3;
 
-		if ((magic == XFS_DIR2_LEAF1_MAGIC &&
-		     leafhdr.magic != XFS_DIR3_LEAF1_MAGIC) ||
-		    (magic == XFS_DIR2_LEAFN_MAGIC &&
-		     leafhdr.magic != XFS_DIR3_LEAFN_MAGIC))
-			return false;
+		magic3 = (magic == XFS_DIR2_LEAF1_MAGIC) ? XFS_DIR3_LEAF1_MAGIC
+							 : XFS_DIR3_LEAFN_MAGIC;
 
+		if (leaf3->info.hdr.magic != cpu_to_be16(magic3))
+			return false;
 		if (!uuid_equal(&leaf3->info.uuid, &mp->m_sb.sb_uuid))
 			return false;
 		if (be64_to_cpu(leaf3->info.blkno) != bp->b_bn)
 			return false;
 	} else {
-		if (leafhdr.magic != magic)
+		if (leaf->hdr.info.magic != cpu_to_be16(magic))
 			return false;
 	}
+
+	xfs_dir3_leaf_hdr_from_disk(&leafhdr, leaf);
 	return xfs_dir3_leaf_check_int(mp, &leafhdr, leaf);
 }
 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [PATCH v2] xfs: check magic numbers in dir3 leaf verifier first
  2013-09-03  0:06 ` [PATCH v2] " Dave Chinner
@ 2013-09-03  7:33   ` Christoph Hellwig
  2013-09-09 22:51   ` Ben Myers
  2013-10-18 16:48   ` Rich Johnston
  2 siblings, 0 replies; 5+ messages in thread
From: Christoph Hellwig @ 2013-09-03  7:33 UTC (permalink / raw)
  To: Dave Chinner; +Cc: xfs

> @@ -165,6 +165,7 @@ xfs_dir3_leaf_check_int(
>  	    (char *)&ents[hdr->count] > (char *)xfs_dir2_leaf_bests_p(ltp))
>  		return false;
>  
> +
>  	/* Check hash value order, count stale entries.  */

spurious new line..

>  	if (xfs_sb_version_hascrc(&mp->m_sb)) {
>  		struct xfs_dir3_leaf_hdr *leaf3 = bp->b_addr;
> +		__uint16_t		magic3;
>  
> +		magic3 = (magic == XFS_DIR2_LEAF1_MAGIC) ? XFS_DIR3_LEAF1_MAGIC
> +							 : XFS_DIR3_LEAFN_MAGIC;
>  
> +		if (leaf3->info.hdr.magic != cpu_to_be16(magic3))
> +			return false;
>  		if (!uuid_equal(&leaf3->info.uuid, &mp->m_sb.sb_uuid))
>  			return false;
>  		if (be64_to_cpu(leaf3->info.blkno) != bp->b_bn)
>  			return false;
>  	} else {
> -		if (leafhdr.magic != magic)
> +		if (leaf->hdr.info.magic != cpu_to_be16(magic))
>  			return false;
>  	}

Looks good but I wonder if something like this should be factored into
a separate helper in the long run.


Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH v2] xfs: check magic numbers in dir3 leaf verifier first
  2013-09-03  0:06 ` [PATCH v2] " Dave Chinner
  2013-09-03  7:33   ` Christoph Hellwig
@ 2013-09-09 22:51   ` Ben Myers
  2013-10-18 16:48   ` Rich Johnston
  2 siblings, 0 replies; 5+ messages in thread
From: Ben Myers @ 2013-09-09 22:51 UTC (permalink / raw)
  To: Dave Chinner; +Cc: xfs

On Tue, Sep 03, 2013 at 10:06:58AM +1000, Dave Chinner wrote:
> xfs: check magic numbers in dir3 leaf verifier first
> 
> From: Dave Chinner <dchinner@redhat.com>
> 
> Calling xfs_dir3_leaf_hdr_from_disk() in a verifier before
> validating the magic numbers in the buffer results in ASSERT
> failures due to mismatching magic numbers when a corruption occurs.
> Seeing as the verifier is supposed to catch the corruption and pass
> it back to the caller, having the verifier assert fail on error
> defeats the purpose of detecting the errors in the first place.
> 
> Check the magic numbers direct from the buffer before decoding the
> header.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Cleaned up the spurious newline and applied.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH v2] xfs: check magic numbers in dir3 leaf verifier first
  2013-09-03  0:06 ` [PATCH v2] " Dave Chinner
  2013-09-03  7:33   ` Christoph Hellwig
  2013-09-09 22:51   ` Ben Myers
@ 2013-10-18 16:48   ` Rich Johnston
  2 siblings, 0 replies; 5+ messages in thread
From: Rich Johnston @ 2013-10-18 16:48 UTC (permalink / raw)
  To: Dave Chinner, xfs

This has been committed.

Thanks
--Rich

commit 36bca8993f52be84f31c476449ca1527d62e16e1
Author: Dave Chinner <dchinner@redhat.com>
Date:   Mon Sep 30 03:15:16 2013 +0000

     xfs: check magic numbers in dir3 leaf verifier first

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-10-18 16:48 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-09-02 10:50 [PATCH] xfs: check magic numbers in dir3 leaf verifier first Dave Chinner
2013-09-03  0:06 ` [PATCH v2] " Dave Chinner
2013-09-03  7:33   ` Christoph Hellwig
2013-09-09 22:51   ` Ben Myers
2013-10-18 16:48   ` Rich Johnston

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.