xfs: more verifications!

From: Dave Chinner <[email protected]>

When stripe alignments are invalid, data alignment algorithms in the allocator may not work correctly. Ensure we catch superblocks with invalid stripe alignment setups at mount time. These data alignment mismatches are now detected at mount time like this:

XFS (loop0): SB stripe unit sanity check failed
XFS (loop0): Metadata corruption detected at xfs_sb_read_verify+0xab/0x110, xfs_sb block 0xffffffffffffffff XFS (loop0): Unmount and run xfs_repair
XFS (loop0): First 128 bytes of corrupted metadata buffer:

0000000091c2de02: 58 46 53 42 00 00 10 00 00 00 00 00 00 00 10 00  XFSB............
0000000023bff869: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
00000000cdd8c893: 17 32 37 15 ff ca 46 3d 9a 17 d3 33 04 b5 f1 a2  .27...F=...3....
000000009fd2844f: 00 00 00 00 00 00 00 04 00 00 00 00 00 00 06 d0  ................
0000000088e9b0bb: 00 00 00 00 00 00 06 d1 00 00 00 00 00 00 06 d2  ................
00000000ff233a20: 00 00 00 01 00 00 10 00 00 00 00 01 00 00 00 00  ................
000000009db0ac8b: 00 00 03 60 e1 34 02 00 08 00 00 02 00 00 00 00  ...`.4..........
00000000f7022460: 00 00 00 00 00 00 00 00 0c 09 0b 01 0c 00 00 19  ................

XFS (loop0): SB validate failed with error -117.

And the mount fails.

Signed-off-by Dave Chinner <[email protected]>

Reviewed-by Darrick J. Wong <[email protected]>

---
 fs/xfs/libxfs/xfs_sb.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

fs/xfs/libxfs/xfs_sb.c +16 -0

index 54c5e14fdfda..fe9448862667 100644
@@ -266,6 +266,22 @@ xfs_mount_validate_sb(
 		return -EFSCORRUPTED;
 	}
 
+	if (sbp->sb_unit) {
+		if (!xfs_sb_version_hasdalign(sbp) ||
+		    sbp->sb_unit > sbp->sb_width ||
+		    (sbp->sb_width % sbp->sb_unit) != 0) {
+			xfs_notice(mp, "SB stripe unit sanity check failed");
+			return -EFSCORRUPTED;
+		}
+	} else if (xfs_sb_version_hasdalign(sbp)) {
+		xfs_notice(mp, "SB stripe alignment sanity check failed");
+		return -EFSCORRUPTED;
+	} else if (sbp->sb_width) {
+		xfs_notice(mp, "SB stripe width sanity check failed");
+		return -EFSCORRUPTED;
+	}
+
+
 	if (xfs_sb_version_hascrc(&mp->m_sb) &&
 	    sbp->sb_blocksize < XFS_MIN_CRC_BLOCKSIZE) {
 		xfs_notice(mp, "v5 SB sanity check failed");

Signature · 6 lines

2.17.0

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Dave Chinner <[email protected]>

There are rules for vald extent size hints. We enforce them when applications set them, but fuzzers violate those rules and that screws us over.

This results in alignment assertion failures when setting up allocations such as this in direct IO:

XFS: Assertion failed: ap->length, file: fs/xfs/libxfs/xfs_bmap.c, line: 3432
....

Call Trace:
 xfs_bmap_btalloc+0x415/0x910
 xfs_bmapi_write+0x71c/0x12e0
 xfs_iomap_write_direct+0x2a9/0x420
 xfs_file_iomap_begin+0x4dc/0xa70
 iomap_apply+0x43/0x100
 iomap_file_buffered_write+0x62/0x90
 xfs_file_buffered_aio_write+0xba/0x300
 __vfs_write+0xd5/0x150
 vfs_write+0xb6/0x180
 ksys_write+0x45/0xa0
 do_syscall_64+0x5a/0x180
 entry_SYSCALL_64_after_hwframe+0x49/0xbe

And from xfs_db:

core.extsize = 10380288

Which is not an integer multiple of the block size, and so violates Rule #7 for setting extent size hints. Validate extent size hint rules in the inode verifier to catch this.

Signed-off-by Dave Chinner <[email protected]>

---
 fs/xfs/libxfs/xfs_inode_buf.c | 7 +++++++
 1 file changed, 7 insertions(+)

fs/xfs/libxfs/xfs_inode_buf.c +0 -0

index f5fff1ccb61d..be197c91307b 100644

fs/xfs/libxfs/xfs_inode_buf.c +6 -0

 	uint16_t		mode;
 	uint16_t		flags;
 	uint64_t		flags2;

@@ -501,6 +502,12 @@ xfs_dinode_verify(
 			return __this_address;
 	}
 
+	/* extent size hint validation */
+	fa = xfs_inode_validate_extsize(mp, be32_to_cpu(dip->di_extsize),
+					mode, be32_to_cpu(dip->di_flags));
+	if (fa)
+		return fa;
+
 	/* only version 3 or greater inodes are extensively verified here */
 	if (dip->di_version < 3)
 		return NULL;

Signature · 6 lines

2.17.0

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 38 lines · Dave Chinner

From: Dave Chinner <[email protected]>

There are rules for vald extent size hints. We enforce them when applications set them, but fuzzers violate those rules and that screws us over.

This results in alignment assertion failures when setting up allocations such as this in direct IO:

XFS: Assertion failed: ap->length, file: fs/xfs/libxfs/xfs_bmap.c, line: 3432
....

Call Trace:
 xfs_bmap_btalloc+0x415/0x910
 xfs_bmapi_write+0x71c/0x12e0
 xfs_iomap_write_direct+0x2a9/0x420
 xfs_file_iomap_begin+0x4dc/0xa70
 iomap_apply+0x43/0x100
 iomap_file_buffered_write+0x62/0x90
 xfs_file_buffered_aio_write+0xba/0x300
 __vfs_write+0xd5/0x150
 vfs_write+0xb6/0x180
 ksys_write+0x45/0xa0
 do_syscall_64+0x5a/0x180
 entry_SYSCALL_64_after_hwframe+0x49/0xbe

And from xfs_db:

core.extsize = 10380288

Which is not an integer multiple of the block size, and so violates Rule #7 for setting extent size hints. Validate extent size hint rules in the inode verifier to catch this.

Signed-off-by Dave Chinner <[email protected]>

---
 fs/xfs/libxfs/xfs_inode_buf.c | 7 +++++++
 1 file changed, 7 insertions(+)

fs/xfs/libxfs/xfs_inode_buf.c xfs_dinode_verify( · line 388

Quoted 9 lines

fs/xfs/libxfs/xfs_inode_buf.c +1 -0

index f5fff1ccb61d..be197c91307b 100644
@@ -385,6 +385,7 @@ xfs_dinode_verify(
 	xfs_ino_t		ino,
 	struct xfs_dinode	*dip)
 {
+	xfs_failaddr_t		fa;

Weren't we getting rid of typedefs?

To be honest the typedef here gives more
clarity to the code than void* directly, so, I'm ok with it anyway, I'm just curious is some typedefs are going to be kept.

From: Dave Chinner <[email protected]>

There are rules for vald extent size hints. We enforce them when applications set them, but fuzzers violate those rules and that screws us over.

This results in alignment assertion failures when setting up allocations such as this in direct IO:

XFS: Assertion failed: ap->length, file: fs/xfs/libxfs/xfs_bmap.c, line: 3432
....

Call Trace:
 xfs_bmap_btalloc+0x415/0x910
 xfs_bmapi_write+0x71c/0x12e0
 xfs_iomap_write_direct+0x2a9/0x420
 xfs_file_iomap_begin+0x4dc/0xa70
 iomap_apply+0x43/0x100
 iomap_file_buffered_write+0x62/0x90
 xfs_file_buffered_aio_write+0xba/0x300
 __vfs_write+0xd5/0x150
 vfs_write+0xb6/0x180
 ksys_write+0x45/0xa0
 do_syscall_64+0x5a/0x180
 entry_SYSCALL_64_after_hwframe+0x49/0xbe

And from xfs_db:

core.extsize = 10380288

Which is not an integer multiple of the block size, and so violates Rule #7 for setting extent size hints. Validate extent size hint rules in the inode verifier to catch this.

Signed-off-by Dave Chinner <[email protected]>

---
 fs/xfs/libxfs/xfs_inode_buf.c | 7 +++++++
 1 file changed, 7 insertions(+)

fs/xfs/libxfs/xfs_inode_buf.c +7 -0

index f5fff1ccb61d..be197c91307b 100644
@@ -385,6 +385,7 @@ xfs_dinode_verify(
 	xfs_ino_t		ino,
 	struct xfs_dinode	*dip)
 {
+	xfs_failaddr_t		fa;
 	uint16_t		mode;
 	uint16_t		flags;
 	uint64_t		flags2;

@@ -501,6 +502,12 @@ xfs_dinode_verify(
 			return __this_address;
 	}
 
+	/* extent size hint validation */
+	fa = xfs_inode_validate_extsize(mp, be32_to_cpu(dip->di_extsize),
+					mode, be32_to_cpu(dip->di_flags));
+	if (fa)
+		return fa;
+
 	/* only version 3 or greater inodes are extensively verified here */
 	if (dip->di_version < 3)
 		return NULL;

Signature · 6 lines

2.17.0

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 29 lines · Dave Chinner

From: Dave Chinner <[email protected]>

There are rules for vald extent size hints. We enforce them when applications set them, but fuzzers violate those rules and that screws us over.

This results in alignment assertion failures when setting up allocations such as this in direct IO:

XFS: Assertion failed: ap->length, file: fs/xfs/libxfs/xfs_bmap.c, line: 3432
....

Call Trace:
 xfs_bmap_btalloc+0x415/0x910
 xfs_bmapi_write+0x71c/0x12e0
 xfs_iomap_write_direct+0x2a9/0x420
 xfs_file_iomap_begin+0x4dc/0xa70
 iomap_apply+0x43/0x100
 iomap_file_buffered_write+0x62/0x90
 xfs_file_buffered_aio_write+0xba/0x300
 __vfs_write+0xd5/0x150
 vfs_write+0xb6/0x180
 ksys_write+0x45/0xa0
 do_syscall_64+0x5a/0x180
 entry_SYSCALL_64_after_hwframe+0x49/0xbe

And from xfs_db:

core.extsize = 10380288

Quoted 5 lines

Which is not an integer multiple of the block size, and so violates Rule #7 for setting extent size hints. Validate extent size hint rules in the inode verifier to catch this.

Signed-off-by Dave Chinner <[email protected]>

Looks ok modulo my comments in the next patch,

Reviewed-by Darrick J. Wong <[email protected]>

--D

Quoted 36 lines

---
 fs/xfs/libxfs/xfs_inode_buf.c | 7 +++++++
 1 file changed, 7 insertions(+)

fs/xfs/libxfs/xfs_inode_buf.c +7 -0

index f5fff1ccb61d..be197c91307b 100644
@@ -385,6 +385,7 @@ xfs_dinode_verify(
 	xfs_ino_t		ino,
 	struct xfs_dinode	*dip)
 {
+	xfs_failaddr_t		fa;
 	uint16_t		mode;
 	uint16_t		flags;
 	uint64_t		flags2;

@@ -501,6 +502,12 @@ xfs_dinode_verify(
 			return __this_address;
 	}
 
+	/* extent size hint validation */
+	fa = xfs_inode_validate_extsize(mp, be32_to_cpu(dip->di_extsize),
+					mode, be32_to_cpu(dip->di_flags));
+	if (fa)
+		return fa;
+
 	/* only version 3 or greater inodes are extensively verified here */
 	if (dip->di_version < 3)
 		return NULL;

Signature · 6 lines

2.17.0

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 30 lines · Darrick J. Wong

Quoted 29 lines · Dave Chinner

From: Dave Chinner <[email protected]>

There are rules for vald extent size hints. We enforce them when applications set them, but fuzzers violate those rules and that screws us over.

This results in alignment assertion failures when setting up allocations such as this in direct IO:

XFS: Assertion failed: ap->length, file: fs/xfs/libxfs/xfs_bmap.c, line: 3432
....

Call Trace:
 xfs_bmap_btalloc+0x415/0x910
 xfs_bmapi_write+0x71c/0x12e0
 xfs_iomap_write_direct+0x2a9/0x420
 xfs_file_iomap_begin+0x4dc/0xa70
 iomap_apply+0x43/0x100
 iomap_file_buffered_write+0x62/0x90
 xfs_file_buffered_aio_write+0xba/0x300
 __vfs_write+0xd5/0x150
 vfs_write+0xb6/0x180
 ksys_write+0x45/0xa0
 do_syscall_64+0x5a/0x180
 entry_SYSCALL_64_after_hwframe+0x49/0xbe

And from xfs_db:

core.extsize = 10380288

Quoted 8 lines

Quoted 5 lines

Which is not an integer multiple of the block size, and so violates Rule #7 for setting extent size hints. Validate extent size hint rules in the inode verifier to catch this.

Signed-off-by Dave Chinner <[email protected]>

Looks ok modulo my comments in the next patch,

Reviewed-by Darrick J. Wong <[email protected]>

FWIW when I applied this to xfsprogs I saw an xfs/033 regression:

Phase 6 - check inode connectivity...
reinitializing root directory
Metadata corruption detected at 0x5555555c60e0, inode 0x80 dinode

fatal error -- could not iget root inode -- error - 117 [Inferior 1 (process 1178) exited with code 01]
(gdb) l *(0x5555555c60e0)
0x5555555c60e0 is in libxfs_inode_validate_extsize (xfs_inode_buf.c:729).

We fail the inode verifier while trying to _iget the root inode so that we can reinitialize it; I suspect phase 3 is going to need to check the extent size hints and clear them.

Quoted 46 lines · Darrick J. Wong

Quoted 30 lines · Darrick J. Wong

Quoted 29 lines · Dave Chinner

From: Dave Chinner <[email protected]>

There are rules for vald extent size hints. We enforce them when applications set them, but fuzzers violate those rules and that screws us over.

This results in alignment assertion failures when setting up allocations such as this in direct IO:

XFS: Assertion failed: ap->length, file: fs/xfs/libxfs/xfs_bmap.c, line: 3432
....

Call Trace:
 xfs_bmap_btalloc+0x415/0x910
 xfs_bmapi_write+0x71c/0x12e0
 xfs_iomap_write_direct+0x2a9/0x420
 xfs_file_iomap_begin+0x4dc/0xa70
 iomap_apply+0x43/0x100
 iomap_file_buffered_write+0x62/0x90
 xfs_file_buffered_aio_write+0xba/0x300
 __vfs_write+0xd5/0x150
 vfs_write+0xb6/0x180
 ksys_write+0x45/0xa0
 do_syscall_64+0x5a/0x180
 entry_SYSCALL_64_after_hwframe+0x49/0xbe

And from xfs_db:

core.extsize = 10380288

Quoted 8 lines

Quoted 5 lines

Which is not an integer multiple of the block size, and so violates Rule #7 for setting extent size hints. Validate extent size hint rules in the inode verifier to catch this.

Signed-off-by Dave Chinner <[email protected]>

Looks ok modulo my comments in the next patch,

Reviewed-by Darrick J. Wong <[email protected]>

FWIW when I applied this to xfsprogs I saw an xfs/033 regression:

Phase 6 - check inode connectivity...
reinitializing root directory
Metadata corruption detected at 0x5555555c60e0, inode 0x80 dinode

Quoted 8 lines

fatal error -- could not iget root inode -- error - 117 [Inferior 1 (process 1178) exited with code 01]
(gdb) l *(0x5555555c60e0)
0x5555555c60e0 is in libxfs_inode_validate_extsize (xfs_inode_buf.c:729).

We fail the inode verifier while trying to _iget the root inode so that we can reinitialize it; I suspect phase 3 is going to need to check the extent size hints and clear them.

I'm actually quite happy to see that the continual process of hardening the kernel verifiers has got to the point where we are starting to expose deficiencies in xfs_repair.

Can I wait for the xfsprogs libxfs-4.18-sync branch to pick up these verifier changes before looking at what repair needs to do to avoid it? I don't want to do a forced context switch to debugging/enhancing userspace code right at this moment....

Cheers,

Dave.

Signature · 6 lines

Dave Chinner
[email protected]
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 56 lines · Dave Chinner

Quoted 46 lines · Darrick J. Wong

Quoted 30 lines · Darrick J. Wong

Quoted 29 lines · Dave Chinner

From: Dave Chinner <[email protected]>

There are rules for vald extent size hints. We enforce them when applications set them, but fuzzers violate those rules and that screws us over.

This results in alignment assertion failures when setting up allocations such as this in direct IO:

XFS: Assertion failed: ap->length, file: fs/xfs/libxfs/xfs_bmap.c, line: 3432
....

Call Trace:
 xfs_bmap_btalloc+0x415/0x910
 xfs_bmapi_write+0x71c/0x12e0
 xfs_iomap_write_direct+0x2a9/0x420
 xfs_file_iomap_begin+0x4dc/0xa70
 iomap_apply+0x43/0x100
 iomap_file_buffered_write+0x62/0x90
 xfs_file_buffered_aio_write+0xba/0x300
 __vfs_write+0xd5/0x150
 vfs_write+0xb6/0x180
 ksys_write+0x45/0xa0
 do_syscall_64+0x5a/0x180
 entry_SYSCALL_64_after_hwframe+0x49/0xbe

And from xfs_db:

core.extsize = 10380288

Quoted 8 lines

Quoted 5 lines

Which is not an integer multiple of the block size, and so violates Rule #7 for setting extent size hints. Validate extent size hint rules in the inode verifier to catch this.

Signed-off-by Dave Chinner <[email protected]>

Looks ok modulo my comments in the next patch,

Reviewed-by Darrick J. Wong <[email protected]>

FWIW when I applied this to xfsprogs I saw an xfs/033 regression:

Phase 6 - check inode connectivity...
reinitializing root directory
Metadata corruption detected at 0x5555555c60e0, inode 0x80 dinode

Quoted 8 lines

fatal error -- could not iget root inode -- error - 117 [Inferior 1 (process 1178) exited with code 01]
(gdb) l *(0x5555555c60e0)
0x5555555c60e0 is in libxfs_inode_validate_extsize (xfs_inode_buf.c:729).

We fail the inode verifier while trying to _iget the root inode so that we can reinitialize it; I suspect phase 3 is going to need to check the extent size hints and clear them.

Quoted 8 lines

I'm actually quite happy to see that the continual process of hardening the kernel verifiers has got to the point where we are starting to expose deficiencies in xfs_repair.

Can I wait for the xfsprogs libxfs-4.18-sync branch to pick up these verifier changes before looking at what repair needs to do to avoid it? I don't want to do a forced context switch to debugging/enhancing userspace code right at this moment....

That's ultimately up to Eric, but since fixing it is nontrivial surgery on xfs_repair (and the verifier update patch doesn't itself break the build) I'd be fine with fixing it after the 4.18 sync goes in.

--D

From: Dave Chinner <[email protected]>

There are rules for vald extent size hints. We enforce them when applications set them, but fuzzers violate those rules and that screws us over.

This results in alignment assertion failures when setting up allocations such as this in direct IO:

XFS: Assertion failed: ap->length, file: fs/xfs/libxfs/xfs_bmap.c, line: 3432
....

Call Trace:
 xfs_bmap_btalloc+0x415/0x910
 xfs_bmapi_write+0x71c/0x12e0
 xfs_iomap_write_direct+0x2a9/0x420
 xfs_file_iomap_begin+0x4dc/0xa70
 iomap_apply+0x43/0x100
 iomap_file_buffered_write+0x62/0x90
 xfs_file_buffered_aio_write+0xba/0x300
 __vfs_write+0xd5/0x150
 vfs_write+0xb6/0x180
 ksys_write+0x45/0xa0
 do_syscall_64+0x5a/0x180
 entry_SYSCALL_64_after_hwframe+0x49/0xbe

And from xfs_db:

core.extsize = 10380288

Which is not an integer multiple of the block size, and so violates Rule #7 for setting extent size hints. Validate extent size hint rules in the inode verifier to catch this.

Quoted 25 lines · Dave Chinner

From: Dave Chinner <[email protected]>

There are rules for vald extent size hints. We enforce them when applications set them, but fuzzers violate those rules and that screws us over.

This results in alignment assertion failures when setting up allocations such as this in direct IO:

XFS: Assertion failed: ap->length, file: fs/xfs/libxfs/xfs_bmap.c, line: 3432
....

Call Trace:
 xfs_bmap_btalloc+0x415/0x910
 xfs_bmapi_write+0x71c/0x12e0
 xfs_iomap_write_direct+0x2a9/0x420
 xfs_file_iomap_begin+0x4dc/0xa70
 iomap_apply+0x43/0x100
 iomap_file_buffered_write+0x62/0x90
 xfs_file_buffered_aio_write+0xba/0x300
 __vfs_write+0xd5/0x150
 vfs_write+0xb6/0x180
 ksys_write+0x45/0xa0
 do_syscall_64+0x5a/0x180
 entry_SYSCALL_64_after_hwframe+0x49/0xbe

Quoted 7 lines

And from xfs_db:

core.extsize = 10380288

Which is not an integer multiple of the block size, and so violates Rule #7 for setting extent size hints. Validate extent size hint rules in the inode verifier to catch this.

So, I think that if I do:

# mkfs.xfs -f -m crc=0 $TEST_DEV
# ./check xfs/229
# ./check xfs/229

I trip the verifier, because I end up with freed inodes on disk with an extent size hints but zeroed flags.

xfs_ifree sets di_flags = 0 but doesn't clear di_extsize; xfs_inode_validate_extsize says if extsize !=0 and the hint flag is set, it fails

Anyone else see this?

(crc=0 needed because that causes us to actually reread the inode chunks in xfs_iread vs. /* shortcut IO on inode allocation if possible */

-Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 40 lines · Eric Sandeen

Quoted 25 lines · Dave Chinner

From: Dave Chinner <[email protected]>

There are rules for vald extent size hints. We enforce them when applications set them, but fuzzers violate those rules and that screws us over.

This results in alignment assertion failures when setting up allocations such as this in direct IO:

XFS: Assertion failed: ap->length, file: fs/xfs/libxfs/xfs_bmap.c, line: 3432
....

Call Trace:
 xfs_bmap_btalloc+0x415/0x910
 xfs_bmapi_write+0x71c/0x12e0
 xfs_iomap_write_direct+0x2a9/0x420
 xfs_file_iomap_begin+0x4dc/0xa70
 iomap_apply+0x43/0x100
 iomap_file_buffered_write+0x62/0x90
 xfs_file_buffered_aio_write+0xba/0x300
 __vfs_write+0xd5/0x150
 vfs_write+0xb6/0x180
 ksys_write+0x45/0xa0
 do_syscall_64+0x5a/0x180
 entry_SYSCALL_64_after_hwframe+0x49/0xbe

Quoted 7 lines

And from xfs_db:

core.extsize = 10380288

Which is not an integer multiple of the block size, and so violates Rule #7 for setting extent size hints. Validate extent size hint rules in the inode verifier to catch this.

So, I think that if I do:

# mkfs.xfs -f -m crc=0 $TEST_DEV
# ./check xfs/229
# ./check xfs/229

Quoted 7 lines

I trip the verifier, because I end up with freed inodes on disk with an extent size hints but zeroed flags.

xfs_ifree sets di_flags = 0 but doesn't clear di_extsize; xfs_inode_validate_extsize says if extsize !=0 and the hint flag is set, it fails

Anyone else see this?

Yeah, I think I just hit this on the TEST_DEV in xfs/242.

git blame says I lifted the code from the scrub code, and I probably wrote the code having read the ioctl code (which clears the extsize field if the iflag isn't set).

Quoted 2 lines

(crc=0 needed because that causes us to actually reread the inode chunks in xfs_iread vs. /* shortcut IO on inode allocation if possible */

Hmmm, so a v5 fs mounted with ikeep will also read an inode chunk when creating an inode. It looks like we do that (instead of zeroing the incore inode and setting a random i_generation) to preserve the existing generation number?

In any case, it's pretty clear that kernels have been writing out freed inode cores with di_mode == 0, di_flags == 0, and di_extsize == (some number) so we clearly can't have that in the verifier. It looks like we only examine di_extsize if either EXTSZ flag are set, so it's not causing incorrect behavior. Maybe it can be a preening fix in scrub/repair.

--D

Quoted 5 lines

-Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 63 lines · Darrick J. Wong

Quoted 40 lines · Eric Sandeen

Quoted 25 lines · Dave Chinner

From: Dave Chinner <[email protected]>

There are rules for vald extent size hints. We enforce them when applications set them, but fuzzers violate those rules and that screws us over.

This results in alignment assertion failures when setting up allocations such as this in direct IO:

XFS: Assertion failed: ap->length, file: fs/xfs/libxfs/xfs_bmap.c, line: 3432
....

Call Trace:
 xfs_bmap_btalloc+0x415/0x910
 xfs_bmapi_write+0x71c/0x12e0
 xfs_iomap_write_direct+0x2a9/0x420
 xfs_file_iomap_begin+0x4dc/0xa70
 iomap_apply+0x43/0x100
 iomap_file_buffered_write+0x62/0x90
 xfs_file_buffered_aio_write+0xba/0x300
 __vfs_write+0xd5/0x150
 vfs_write+0xb6/0x180
 ksys_write+0x45/0xa0
 do_syscall_64+0x5a/0x180
 entry_SYSCALL_64_after_hwframe+0x49/0xbe

Quoted 7 lines

And from xfs_db:

core.extsize = 10380288

Which is not an integer multiple of the block size, and so violates Rule #7 for setting extent size hints. Validate extent size hint rules in the inode verifier to catch this.

So, I think that if I do:

# mkfs.xfs -f -m crc=0 $TEST_DEV
# ./check xfs/229
# ./check xfs/229

Quoted 7 lines

I trip the verifier, because I end up with freed inodes on disk with an extent size hints but zeroed flags.

xfs_ifree sets di_flags = 0 but doesn't clear di_extsize; xfs_inode_validate_extsize says if extsize !=0 and the hint flag is set, it fails

Anyone else see this?

Yeah, I think I just hit this on the TEST_DEV in xfs/242.

git blame says I lifted the code from the scrub code, and I probably wrote the code having read the ioctl code (which clears the extsize field if the iflag isn't set).

Quoted 2 lines

(crc=0 needed because that causes us to actually reread the inode chunks in xfs_iread vs. /* shortcut IO on inode allocation if possible */

Hmmm, so a v5 fs mounted with ikeep will also read an inode chunk when creating an inode. It looks like we do that (instead of zeroing the incore inode and setting a random i_generation) to preserve the existing generation number?

Quoted 7 lines

In any case, it's pretty clear that kernels have been writing out freed inode cores with di_mode == 0, di_flags == 0, and di_extsize == (some number) so we clearly can't have that in the verifier. It looks like we only examine di_extsize if either EXTSZ flag are set, so it's not causing incorrect behavior. Maybe it can be a preening fix in scrub/repair.

I just stumbled on this problem with xfs/229 that Eric reported. I'm confused by the comment above regarding this not causing incorrect behavior.

Quoted 72 lines · Brian Foster

Quoted 70 lines · Darrick J. Wong

Quoted 40 lines · Eric Sandeen

Quoted 25 lines · Dave Chinner

From: Dave Chinner <[email protected]>

There are rules for vald extent size hints. We enforce them when applications set them, but fuzzers violate those rules and that screws us over.

This results in alignment assertion failures when setting up allocations such as this in direct IO:

XFS: Assertion failed: ap->length, file: fs/xfs/libxfs/xfs_bmap.c, line: 3432
....

Call Trace:
 xfs_bmap_btalloc+0x415/0x910
 xfs_bmapi_write+0x71c/0x12e0
 xfs_iomap_write_direct+0x2a9/0x420
 xfs_file_iomap_begin+0x4dc/0xa70
 iomap_apply+0x43/0x100
 iomap_file_buffered_write+0x62/0x90
 xfs_file_buffered_aio_write+0xba/0x300
 __vfs_write+0xd5/0x150
 vfs_write+0xb6/0x180
 ksys_write+0x45/0xa0
 do_syscall_64+0x5a/0x180
 entry_SYSCALL_64_after_hwframe+0x49/0xbe

Quoted 7 lines

And from xfs_db:

core.extsize = 10380288

Which is not an integer multiple of the block size, and so violates Rule #7 for setting extent size hints. Validate extent size hint rules in the inode verifier to catch this.

So, I think that if I do:

# mkfs.xfs -f -m crc=0 $TEST_DEV
# ./check xfs/229
# ./check xfs/229

Quoted 7 lines

I trip the verifier, because I end up with freed inodes on disk with an extent size hints but zeroed flags.

xfs_ifree sets di_flags = 0 but doesn't clear di_extsize; xfs_inode_validate_extsize says if extsize !=0 and the hint flag is set, it fails

Anyone else see this?

Yeah, I think I just hit this on the TEST_DEV in xfs/242.

git blame says I lifted the code from the scrub code, and I probably wrote the code having read the ioctl code (which clears the extsize field if the iflag isn't set).

Quoted 2 lines

(crc=0 needed because that causes us to actually reread the inode chunks in xfs_iread vs. /* shortcut IO on inode allocation if possible */

Hmmm, so a v5 fs mounted with ikeep will also read an inode chunk when creating an inode. It looks like we do that (instead of zeroing the incore inode and setting a random i_generation) to preserve the existing generation number?

In any case, it's pretty clear that kernels have been writing out freed inode cores with di_mode == 0, di_flags == 0, and di_extsize == (some number) so we clearly can't have that in the verifier. It looks like we only examine di_extsize if either EXTSZ flag are set, so it's not causing incorrect behavior. Maybe it can be a preening fix in scrub/repair.

Quoted 3 lines

I just stumbled on this problem with xfs/229 that Eric reported. I'm confused by the comment above regarding this not causing incorrect behavior.

I think Darrick meant that having a nonzero extent size hint on disk won't cause incorrect behavior because "we only examine di_extsize if either EXTSZ flag are set"

-Eric

Quoted 79 lines · Eric Sandeen

Quoted 75 lines · Brian Foster

Quoted 63 lines · Darrick J. Wong

Quoted 40 lines · Eric Sandeen

Quoted 25 lines · Dave Chinner

From: Dave Chinner <[email protected]>

There are rules for vald extent size hints. We enforce them when applications set them, but fuzzers violate those rules and that screws us over.

This results in alignment assertion failures when setting up allocations such as this in direct IO:

XFS: Assertion failed: ap->length, file: fs/xfs/libxfs/xfs_bmap.c, line: 3432
....

Call Trace:
 xfs_bmap_btalloc+0x415/0x910
 xfs_bmapi_write+0x71c/0x12e0
 xfs_iomap_write_direct+0x2a9/0x420
 xfs_file_iomap_begin+0x4dc/0xa70
 iomap_apply+0x43/0x100
 iomap_file_buffered_write+0x62/0x90
 xfs_file_buffered_aio_write+0xba/0x300
 __vfs_write+0xd5/0x150
 vfs_write+0xb6/0x180
 ksys_write+0x45/0xa0
 do_syscall_64+0x5a/0x180
 entry_SYSCALL_64_after_hwframe+0x49/0xbe

Quoted 7 lines

And from xfs_db:

core.extsize = 10380288

Which is not an integer multiple of the block size, and so violates Rule #7 for setting extent size hints. Validate extent size hint rules in the inode verifier to catch this.

So, I think that if I do:

# mkfs.xfs -f -m crc=0 $TEST_DEV
# ./check xfs/229
# ./check xfs/229

Quoted 7 lines

I trip the verifier, because I end up with freed inodes on disk with an extent size hints but zeroed flags.

xfs_ifree sets di_flags = 0 but doesn't clear di_extsize; xfs_inode_validate_extsize says if extsize !=0 and the hint flag is set, it fails

Anyone else see this?

Yeah, I think I just hit this on the TEST_DEV in xfs/242.

git blame says I lifted the code from the scrub code, and I probably wrote the code having read the ioctl code (which clears the extsize field if the iflag isn't set).

Quoted 2 lines

(crc=0 needed because that causes us to actually reread the inode chunks in xfs_iread vs. /* shortcut IO on inode allocation if possible */

Hmmm, so a v5 fs mounted with ikeep will also read an inode chunk when creating an inode. It looks like we do that (instead of zeroing the incore inode and setting a random i_generation) to preserve the existing generation number?

Quoted 7 lines

In any case, it's pretty clear that kernels have been writing out freed inode cores with di_mode == 0, di_flags == 0, and di_extsize == (some number) so we clearly can't have that in the verifier. It looks like we only examine di_extsize if either EXTSZ flag are set, so it's not causing incorrect behavior. Maybe it can be a preening fix in scrub/repair.

I just stumbled on this problem with xfs/229 that Eric reported. I'm confused by the comment above regarding this not causing incorrect behavior.

Quoted 3 lines

I think Darrick meant that having a nonzero extent size hint on disk won't cause incorrect behavior because "we only examine di_extsize if either EXTSZ flag are set"

Yeah, he probably did. :)

I think Brian's suggestion of

if (i_mode != 0 && !hint && extsize != 0)

	barf_error();

sounds reasonable (having not tested that at all).

--D

Quoted 1 line

-Eric

Quoted 78 lines · Darrick J. Wong

Quoted 77 lines · Eric Sandeen

Quoted 75 lines · Brian Foster

Quoted 63 lines · Darrick J. Wong

Quoted 40 lines · Eric Sandeen

Quoted 25 lines · Dave Chinner

From: Dave Chinner <[email protected]>

There are rules for vald extent size hints. We enforce them when applications set them, but fuzzers violate those rules and that screws us over.

This results in alignment assertion failures when setting up allocations such as this in direct IO:

XFS: Assertion failed: ap->length, file: fs/xfs/libxfs/xfs_bmap.c, line: 3432
....

Call Trace:
 xfs_bmap_btalloc+0x415/0x910
 xfs_bmapi_write+0x71c/0x12e0
 xfs_iomap_write_direct+0x2a9/0x420
 xfs_file_iomap_begin+0x4dc/0xa70
 iomap_apply+0x43/0x100
 iomap_file_buffered_write+0x62/0x90
 xfs_file_buffered_aio_write+0xba/0x300
 __vfs_write+0xd5/0x150
 vfs_write+0xb6/0x180
 ksys_write+0x45/0xa0
 do_syscall_64+0x5a/0x180
 entry_SYSCALL_64_after_hwframe+0x49/0xbe

Quoted 7 lines

And from xfs_db:

core.extsize = 10380288

Which is not an integer multiple of the block size, and so violates Rule #7 for setting extent size hints. Validate extent size hint rules in the inode verifier to catch this.

So, I think that if I do:

# mkfs.xfs -f -m crc=0 $TEST_DEV
# ./check xfs/229
# ./check xfs/229

Quoted 7 lines

I trip the verifier, because I end up with freed inodes on disk with an extent size hints but zeroed flags.

xfs_ifree sets di_flags = 0 but doesn't clear di_extsize; xfs_inode_validate_extsize says if extsize !=0 and the hint flag is set, it fails

Anyone else see this?

Yeah, I think I just hit this on the TEST_DEV in xfs/242.

git blame says I lifted the code from the scrub code, and I probably wrote the code having read the ioctl code (which clears the extsize field if the iflag isn't set).

Quoted 2 lines

(crc=0 needed because that causes us to actually reread the inode chunks in xfs_iread vs. /* shortcut IO on inode allocation if possible */

Hmmm, so a v5 fs mounted with ikeep will also read an inode chunk when creating an inode. It looks like we do that (instead of zeroing the incore inode and setting a random i_generation) to preserve the existing generation number?

Quoted 7 lines

In any case, it's pretty clear that kernels have been writing out freed inode cores with di_mode == 0, di_flags == 0, and di_extsize == (some number) so we clearly can't have that in the verifier. It looks like we only examine di_extsize if either EXTSZ flag are set, so it's not causing incorrect behavior. Maybe it can be a preening fix in scrub/repair.

I just stumbled on this problem with xfs/229 that Eric reported. I'm confused by the comment above regarding this not causing incorrect behavior.

Quoted 6 lines

Quoted 3 lines

I think Darrick meant that having a nonzero extent size hint on disk won't cause incorrect behavior because "we only examine di_extsize if either EXTSZ flag are set"

Yeah, he probably did. :)

Got it, thanks.

Quoted 5 lines

I think Brian's suggestion of

if (i_mode != 0 && !hint && extsize != 0)

	barf_error();

Quoted 2 lines

sounds reasonable (having not tested that at all).

I'll run it through xfstests and get it posted if nothing else fails.

BTW, do we have a similar issue with the cowextsize hint (assuming v5+ikeep)? It looks like it's cleared similarly in xfs_ialloc(), but I'm not sure if it's cleared somewhere else on free...

Quoted 93 lines · Brian Foster

Quoted 78 lines · Darrick J. Wong

Quoted 77 lines · Eric Sandeen

Quoted 75 lines · Brian Foster

Quoted 63 lines · Darrick J. Wong

Quoted 40 lines · Eric Sandeen

Quoted 25 lines · Dave Chinner

From: Dave Chinner <[email protected]>

There are rules for vald extent size hints. We enforce them when applications set them, but fuzzers violate those rules and that screws us over.

This results in alignment assertion failures when setting up allocations such as this in direct IO:

XFS: Assertion failed: ap->length, file: fs/xfs/libxfs/xfs_bmap.c, line: 3432
....

Call Trace:
 xfs_bmap_btalloc+0x415/0x910
 xfs_bmapi_write+0x71c/0x12e0
 xfs_iomap_write_direct+0x2a9/0x420
 xfs_file_iomap_begin+0x4dc/0xa70
 iomap_apply+0x43/0x100
 iomap_file_buffered_write+0x62/0x90
 xfs_file_buffered_aio_write+0xba/0x300
 __vfs_write+0xd5/0x150
 vfs_write+0xb6/0x180
 ksys_write+0x45/0xa0
 do_syscall_64+0x5a/0x180
 entry_SYSCALL_64_after_hwframe+0x49/0xbe

Quoted 7 lines

And from xfs_db:

core.extsize = 10380288

Which is not an integer multiple of the block size, and so violates Rule #7 for setting extent size hints. Validate extent size hint rules in the inode verifier to catch this.

So, I think that if I do:

# mkfs.xfs -f -m crc=0 $TEST_DEV
# ./check xfs/229
# ./check xfs/229

Quoted 7 lines

I trip the verifier, because I end up with freed inodes on disk with an extent size hints but zeroed flags.

xfs_ifree sets di_flags = 0 but doesn't clear di_extsize; xfs_inode_validate_extsize says if extsize !=0 and the hint flag is set, it fails

Anyone else see this?

Yeah, I think I just hit this on the TEST_DEV in xfs/242.

git blame says I lifted the code from the scrub code, and I probably wrote the code having read the ioctl code (which clears the extsize field if the iflag isn't set).

Quoted 2 lines

(crc=0 needed because that causes us to actually reread the inode chunks in xfs_iread vs. /* shortcut IO on inode allocation if possible */

Hmmm, so a v5 fs mounted with ikeep will also read an inode chunk when creating an inode. It looks like we do that (instead of zeroing the incore inode and setting a random i_generation) to preserve the existing generation number?

Quoted 7 lines

In any case, it's pretty clear that kernels have been writing out freed inode cores with di_mode == 0, di_flags == 0, and di_extsize == (some number) so we clearly can't have that in the verifier. It looks like we only examine di_extsize if either EXTSZ flag are set, so it's not causing incorrect behavior. Maybe it can be a preening fix in scrub/repair.

I just stumbled on this problem with xfs/229 that Eric reported. I'm confused by the comment above regarding this not causing incorrect behavior.

Quoted 6 lines

Quoted 3 lines

I think Darrick meant that having a nonzero extent size hint on disk won't cause incorrect behavior because "we only examine di_extsize if either EXTSZ flag are set"

Yeah, he probably did. :)

Got it, thanks.

Quoted 5 lines

I think Brian's suggestion of

if (i_mode != 0 && !hint && extsize != 0)

	barf_error();

Quoted 8 lines

Quoted 2 lines

sounds reasonable (having not tested that at all).

I'll run it through xfstests and get it posted if nothing else fails.

BTW, do we have a similar issue with the cowextsize hint (assuming v5+ikeep)? It looks like it's cleared similarly in xfs_ialloc(), but I'm not sure if it's cleared somewhere else on free...

We should clear them on free now, so that we can draw a line in the sand for when we can have verifiers check it. e.g. when the next feature bit gets introduced, filesystems with that feature bit set can also verify the extent size hints are zero on freed inodes because we know that kernels supporting that feature always zero them on free....

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Quoted 7 lines · Dave Chinner

We should clear them on free now, so that we can draw a line in the sand for when we can have verifiers check it. e.g. when the next feature bit gets introduced, filesystems with that feature bit set can also verify the extent size hints are zero on freed inodes because we know that kernels supporting that feature always zero them on free....

That seems fine (and harmless) to me if the goal is ultimately to have this content clear on-disk. It keeps things consistent for verifiers, scrub, repair, etc. to not have some bits with required initialized values and others where we need to accommodate stale data.

From: Dave Chinner <[email protected]>

There are rules for vald extent size hints. We enforce them when applications set them, but fuzzers violate those rules and that screws us over. Validate COW extent size hint rules in the inode verifier to catch this.

Signed-off-by Dave Chinner <[email protected]>

---
 fs/xfs/libxfs/xfs_inode_buf.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

fs/xfs/libxfs/xfs_inode_buf.c +8 -2

index be197c91307b..ea64be7cbd98 100644
@@ -504,7 +504,7 @@ xfs_dinode_verify(
 
 	/* extent size hint validation */
 	fa = xfs_inode_validate_extsize(mp, be32_to_cpu(dip->di_extsize),
-					mode, be32_to_cpu(dip->di_flags));
+					mode, flags);
 	if (fa)
 		return fa;
 

@@ -516,7 +516,7 @@ xfs_dinode_verify(
 
 	/* don't allow reflink/cowextsize if we don't have reflink */
 	if ((flags2 & (XFS_DIFLAG2_REFLINK | XFS_DIFLAG2_COWEXTSIZE)) &&
-            !xfs_sb_version_hasreflink(&mp->m_sb))
+	     !xfs_sb_version_hasreflink(&mp->m_sb))
 		return __this_address;
 
 	/* only regular files get reflink */

@@ -531,6 +531,12 @@ xfs_dinode_verify(
 	if ((flags2 & XFS_DIFLAG2_REFLINK) && (flags2 & XFS_DIFLAG2_DAX))
 		return __this_address;
 
+	/* COW extent size hint validation */
+	fa = xfs_inode_validate_cowextsize(mp, be32_to_cpu(dip->di_cowextsize),
+					mode, flags, flags2);
+	if (fa)
+		return fa;
+
 	return NULL;
 }
 

Signature · 6 lines

2.17.0

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Dave Chinner <[email protected]>

There are rules for vald extent size hints. We enforce them when applications set them, but fuzzers violate those rules and that screws us over. Validate COW extent size hint rules in the inode verifier to catch this.

Signed-off-by Dave Chinner <[email protected]>

---
 fs/xfs/libxfs/xfs_inode_buf.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

fs/xfs/libxfs/xfs_inode_buf.c +1 -1

index be197c91307b..ea64be7cbd98 100644
@@ -504,7 +504,7 @@ xfs_dinode_verify(
 
 	/* extent size hint validation */
 	fa = xfs_inode_validate_extsize(mp, be32_to_cpu(dip->di_extsize),
-					mode, be32_to_cpu(dip->di_flags));
+					mode, flags);
 	if (fa)
 		return fa;
 

fs/xfs/libxfs/xfs_inode_buf.c +6 -0

 		return __this_address;
 
 	/* only regular files get reflink */

@@ -531,6 +531,12 @@ xfs_dinode_verify(
 	if ((flags2 & XFS_DIFLAG2_REFLINK) && (flags2 & XFS_DIFLAG2_DAX))
 		return __this_address;
 
+	/* COW extent size hint validation */
+	fa = xfs_inode_validate_cowextsize(mp, be32_to_cpu(dip->di_cowextsize),
+					mode, flags, flags2);
+	if (fa)
+		return fa;
+
 	return NULL;
 }
 

Signature · 6 lines

2.17.0

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

fs/xfs/libxfs/xfs_alloc.c +18 -2

 	union xfs_btree_rec	*rec;
 	int			error;

@@ -222,12 +224,28 @@ xfs_alloc_get_rec(
 	if (error || !(*stat))
 		return error;
 	if (rec->alloc.ar_blockcount == 0)
-		return -EFSCORRUPTED;
+		goto out_bad_rec;
 
 	*bno = be32_to_cpu(rec->alloc.ar_startblock);
 	*len = be32_to_cpu(rec->alloc.ar_blockcount);
 
-	return error;
+	/* check for valid extent range, including overflow */
+	if (!xfs_verify_agbno(mp, agno, *bno))
+		goto out_bad_rec;
+	if (*bno > *bno + *len)
+		goto out_bad_rec;
+	if (!xfs_verify_agbno(mp, agno, *bno + *len - 1))
+		goto out_bad_rec;
+
+	return 0;
+
+out_bad_rec:
+	xfs_warn(mp,
+		"%s Freespace BTree record corruption in AG %d detected!",
+		cur->bc_btnum == XFS_BTNUM_BNO ? "Block" : "Size", agno);
+	xfs_warn(mp,
+		"start block 0x%x block count 0x%x", *bno, *len);
+	return -EFSCORRUPTED;
 }
 
 /*

fs/xfs/libxfs/xfs_ialloc.c +30 -1

index ec5ea02b5553..3f551eb29157 100644
@@ -121,16 +121,45 @@ xfs_inobt_get_rec(
 	struct xfs_inobt_rec_incore	*irec,
 	int				*stat)
 {
+	struct xfs_mount		*mp = cur->bc_mp;
+	xfs_agnumber_t			agno = cur->bc_private.a.agno;
 	union xfs_btree_rec		*rec;
 	int				error;
+	uint64_t			realfree;
 
 	error = xfs_btree_get_rec(cur, &rec, stat);
 	if (error || *stat == 0)
 		return error;
 
-	xfs_inobt_btrec_to_irec(cur->bc_mp, rec, irec);
+	xfs_inobt_btrec_to_irec(mp, rec, irec);
+
+	if (!xfs_verify_agino(mp, agno, irec->ir_startino))
+		goto out_bad_rec;
+	if (irec->ir_count < XFS_INODES_PER_HOLEMASK_BIT ||
+	    irec->ir_count > XFS_INODES_PER_CHUNK)
+		goto out_bad_rec;
+	if (irec->ir_freecount > XFS_INODES_PER_CHUNK)
+		goto out_bad_rec;
+
+	/* if there are no holes, return the first available offset */
+	if (!xfs_inobt_issparse(irec->ir_holemask))
+		realfree = irec->ir_free;
+	else
+		realfree = irec->ir_free & xfs_inobt_irec_to_allocmask(irec);
+	if (hweight64(realfree) != irec->ir_freecount)
+		goto out_bad_rec;
 
 	return 0;
+
+out_bad_rec:
+	xfs_warn(mp,
+		"%s Inode BTree record corruption in AG %d detected!",
+		cur->bc_btnum == XFS_BTNUM_INO ? "Used" : "Free", agno);
+	xfs_warn(mp,
+"start inode 0x%x, count 0x%x, free 0x%x freemask 0x%llx, holemask 0x%x",
+		irec->ir_startino, irec->ir_count, irec->ir_freecount,
+		irec->ir_free, irec->ir_holemask);
+	return -EFSCORRUPTED;
 }
 
 /*

fs/xfs/libxfs/xfs_refcount.c +15 -4

index ed5704c7dcf5..9a2a2004af24 100644
@@ -111,16 +111,51 @@ xfs_refcount_get_rec(
 	struct xfs_refcount_irec	*irec,
 	int				*stat)
 {
+	struct xfs_mount		*mp = cur->bc_mp;
+	xfs_agnumber_t			agno = cur->bc_private.a.agno;
 	union xfs_btree_rec		*rec;
 	int				error;
+	xfs_agblock_t			realstart;
 
 	error = xfs_btree_get_rec(cur, &rec, stat);
-	if (!error && *stat == 1) {
-		xfs_refcount_btrec_to_irec(rec, irec);
-		trace_xfs_refcount_get(cur->bc_mp, cur->bc_private.a.agno,
-				irec);
+	if (error || !*stat)
+		return error;
+
+	xfs_refcount_btrec_to_irec(rec, irec);
+
+	agno = cur->bc_private.a.agno;
+	if (irec->rc_blockcount == 0 || irec->rc_blockcount > MAXREFCEXTLEN)
+		goto out_bad_rec;
+
+	/* handle special COW-staging state */
+	realstart = irec->rc_startblock;
+	if (realstart & XFS_REFC_COW_START) {

Quoted 10 lines

fs/xfs/libxfs/xfs_refcount.c +9 -0

+	if (irec->rc_blockcount == 0 || irec->rc_blockcount > MAXREFCEXTLEN)
+		goto out_bad_rec;
+
+	/* handle special COW-staging state */
+	realstart = irec->rc_startblock;
+	if (realstart & XFS_REFC_COW_START) {
+		if (irec->rc_refcount != 1)
+			goto out_bad_rec;
+		realstart &= ~XFS_REFC_COW_START;
 	}

fs/xfs/libxfs/xfs_refcount.c xfs_refcount_get_rec( · line 141

Quoted 6 lines

fs/xfs/libxfs/xfs_refcount.c +5 -1

-	return error;
+
+	/* check for valid extent range, including overflow */
+	if (!xfs_verify_agbno(mp, agno, realstart))
+		goto out_bad_rec;
+	if (realstart > realstart + irec->rc_blockcount)

I am not sure if I'm right, but I thought this ought to be ">="?

From: Dave Chinner <[email protected]>

So we don't check the validity of records as we walk the btree. When there are corrupt records in the free space btree (e.g. zero startblock/length or beyond EOAG) we just blindly use it and things go bad from there. That leads to assert failures on debug kernels like this:

XFS: Assertion failed: fs_is_ok, file: fs/xfs/libxfs/xfs_alloc.c, line: 450
....

Call Trace:
 xfs_alloc_fixup_trees+0x368/0x5c0
 xfs_alloc_ag_vextent_near+0x79a/0xe20
 xfs_alloc_ag_vextent+0x1d3/0x330
 xfs_alloc_vextent+0x5e9/0x870

Or crashes like this:

XFS (loop0): xfs_buf_find: daddr 0x7fb28 out of range, EOFS 0x8000
.....

BUG: unable to handle kernel NULL pointer dereference at 00000000000000c8
....
Call Trace:
 xfs_bmap_add_extent_hole_real+0x67d/0x930
 xfs_bmapi_write+0x934/0xc90
 xfs_da_grow_inode_int+0x27e/0x2f0
 xfs_dir2_grow_inode+0x55/0x130
 xfs_dir2_sf_to_block+0x94/0x5d0
 xfs_dir2_sf_addname+0xd0/0x590
 xfs_dir_createname+0x168/0x1a0
 xfs_rename+0x658/0x9b0

By checking that free space records pulled from the trees are within the valid range, we catch many of these corruptions before they can do damage.

This is a generic btree record checking deficiency. We need to validate the records we fetch from all the different btrees before we use them to catch corruptions like this.

This patch results in a corrupt record emitting an error message and returning -EFSCORRUPTED, and the higher layers catch that and abort:

XFS (loop0): Size Freespace BTree record corruption in AG 0 detected!
XFS (loop0): start block 0x0 block count 0x0
XFS (loop0): Internal error xfs_trans_cancel at line 1012 of file fs/xfs/xfs_trans.c. Caller xfs_create+0x42a/0x670
.....

 Call Trace:
  dump_stack+0x85/0xcb
  xfs_trans_cancel+0x19f/0x1c0
  xfs_create+0x42a/0x670
  xfs_generic_create+0x1f6/0x2c0
  vfs_create+0xf9/0x180
  do_mknodat+0x1f9/0x210
  do_syscall_64+0x5a/0x180
  entry_SYSCALL_64_after_hwframe+0x49/0xbe

.....
XFS (loop0): xfs_do_force_shutdown(0x8) called from line 1013 of file fs/xfs/xfs_trans.c. Return address = ffffffff81500868 XFS (loop0): Corruption of in-memory data detected. Shutting down filesystem

Signed-off-by Dave Chinner <[email protected]>

---
V2:
- now with overflow checks
- slightly more robust rmap checks that validate AG header region properly.

 fs/xfs/libxfs/xfs_alloc.c    | 22 ++++++++++++++++++++--
 fs/xfs/libxfs/xfs_ialloc.c   | 31 +++++++++++++++++++++++++++++-
 fs/xfs/libxfs/xfs_refcount.c | 45 +++++++++++++++++++++++++++++++++++++++-----
 fs/xfs/libxfs/xfs_rmap.c     | 40 ++++++++++++++++++++++++++++++++++++++-
 4 files changed, 129 insertions(+), 9 deletions(-)

fs/xfs/libxfs/xfs_alloc.c +20 -2

index a52ffb835b16..1db50cfc0212 100644
@@ -215,6 +215,8 @@ xfs_alloc_get_rec(
 	xfs_extlen_t		*len,	/* output: length of extent */
 	int			*stat)	/* output: success/failure */
 {
+	struct xfs_mount	*mp = cur->bc_mp;
+	xfs_agnumber_t		agno = cur->bc_private.a.agno;
 	union xfs_btree_rec	*rec;
 	int			error;
 

@@ -222,12 +224,28 @@ xfs_alloc_get_rec(
 	if (error || !(*stat))
 		return error;
 	if (rec->alloc.ar_blockcount == 0)
-		return -EFSCORRUPTED;
+		goto out_bad_rec;
 
 	*bno = be32_to_cpu(rec->alloc.ar_startblock);
 	*len = be32_to_cpu(rec->alloc.ar_blockcount);
 
-	return error;
+	/* check for valid extent range, including overflow */
+	if (!xfs_verify_agbno(mp, agno, *bno))
+		goto out_bad_rec;
+	if (*bno > *bno + *len)
+		goto out_bad_rec;
+	if (!xfs_verify_agbno(mp, agno, *bno + *len - 1))
+		goto out_bad_rec;
+
+	return 0;
+
+out_bad_rec:
+	xfs_warn(mp,
+		"%s Freespace BTree record corruption in AG %d detected!",
+		cur->bc_btnum == XFS_BTNUM_BNO ? "Block" : "Size", agno);
+	xfs_warn(mp,
+		"start block 0x%x block count 0x%x", *bno, *len);
+	return -EFSCORRUPTED;
 }
 
 /*

fs/xfs/libxfs/xfs_ialloc.c +30 -1

index ec5ea02b5553..3f551eb29157 100644
@@ -121,16 +121,45 @@ xfs_inobt_get_rec(
 	struct xfs_inobt_rec_incore	*irec,
 	int				*stat)
 {
+	struct xfs_mount		*mp = cur->bc_mp;
+	xfs_agnumber_t			agno = cur->bc_private.a.agno;
 	union xfs_btree_rec		*rec;
 	int				error;
+	uint64_t			realfree;
 
 	error = xfs_btree_get_rec(cur, &rec, stat);
 	if (error || *stat == 0)
 		return error;
 
-	xfs_inobt_btrec_to_irec(cur->bc_mp, rec, irec);
+	xfs_inobt_btrec_to_irec(mp, rec, irec);
+
+	if (!xfs_verify_agino(mp, agno, irec->ir_startino))
+		goto out_bad_rec;
+	if (irec->ir_count < XFS_INODES_PER_HOLEMASK_BIT ||
+	    irec->ir_count > XFS_INODES_PER_CHUNK)
+		goto out_bad_rec;
+	if (irec->ir_freecount > XFS_INODES_PER_CHUNK)
+		goto out_bad_rec;
+
+	/* if there are no holes, return the first available offset */
+	if (!xfs_inobt_issparse(irec->ir_holemask))
+		realfree = irec->ir_free;
+	else
+		realfree = irec->ir_free & xfs_inobt_irec_to_allocmask(irec);
+	if (hweight64(realfree) != irec->ir_freecount)
+		goto out_bad_rec;
 
 	return 0;
+
+out_bad_rec:
+	xfs_warn(mp,
+		"%s Inode BTree record corruption in AG %d detected!",
+		cur->bc_btnum == XFS_BTNUM_INO ? "Used" : "Free", agno);
+	xfs_warn(mp,
+"start inode 0x%x, count 0x%x, free 0x%x freemask 0x%llx, holemask 0x%x",
+		irec->ir_startino, irec->ir_count, irec->ir_freecount,
+		irec->ir_free, irec->ir_holemask);
+	return -EFSCORRUPTED;
 }
 
 /*

fs/xfs/libxfs/xfs_refcount.c +9 -4

index ed5704c7dcf5..9a2a2004af24 100644
@@ -111,16 +111,51 @@ xfs_refcount_get_rec(
 	struct xfs_refcount_irec	*irec,
 	int				*stat)
 {
+	struct xfs_mount		*mp = cur->bc_mp;
+	xfs_agnumber_t			agno = cur->bc_private.a.agno;
 	union xfs_btree_rec		*rec;
 	int				error;
+	xfs_agblock_t			realstart;
 
 	error = xfs_btree_get_rec(cur, &rec, stat);
-	if (!error && *stat == 1) {
-		xfs_refcount_btrec_to_irec(rec, irec);
-		trace_xfs_refcount_get(cur->bc_mp, cur->bc_private.a.agno,
-				irec);
+	if (error || !*stat)
+		return error;
+
+	xfs_refcount_btrec_to_irec(rec, irec);
+
+	agno = cur->bc_private.a.agno;

fs/xfs/libxfs/xfs_refcount.c +22 -1

-	return error;
+
+	/* check for valid extent range, including overflow */
+	if (!xfs_verify_agbno(mp, agno, realstart))
+		goto out_bad_rec;
+	if (realstart > realstart + irec->rc_blockcount)
+		goto out_bad_rec;
+	if (!xfs_verify_agbno(mp, agno, realstart + irec->rc_blockcount - 1))
+		goto out_bad_rec;
+
+	if (irec->rc_refcount == 0 || irec->rc_refcount > MAXREFCOUNT)
+		goto out_bad_rec;
+
+	trace_xfs_refcount_get(cur->bc_mp, cur->bc_private.a.agno, irec);
+	return 0;
+
+out_bad_rec:
+	xfs_warn(mp,
+		"Refcount BTree record corruption in AG %d detected!", agno);
+	xfs_warn(mp,
+		"Start block 0x%x, block count 0x%x, references 0x%x",
+		irec->rc_startblock, irec->rc_blockcount, irec->rc_refcount);
+	return -EFSCORRUPTED;
 }
 
 /*

fs/xfs/libxfs/xfs_rmap.c +18 -1

index 87711f9af625..e794bcd6591a 100644
@@ -191,6 +191,8 @@ xfs_rmap_get_rec(
 	struct xfs_rmap_irec	*irec,
 	int			*stat)
 {
+	struct xfs_mount	*mp = cur->bc_mp;
+	xfs_agnumber_t		agno = cur->bc_private.a.agno;
 	union xfs_btree_rec	*rec;
 	int			error;
 

@@ -198,7 +200,43 @@ xfs_rmap_get_rec(
 	if (error || !*stat)
 		return error;
 
-	return xfs_rmap_btrec_to_irec(rec, irec);
+	if (xfs_rmap_btrec_to_irec(rec, irec))
+		goto out_bad_rec;
+
+	if (irec->rm_blockcount == 0)
+		goto out_bad_rec;
+	if (irec->rm_startblock <= XFS_AGFL_BLOCK(mp)) {
+		if (irec->rm_owner != XFS_RMAP_OWN_FS)
+			goto out_bad_rec;
+		if (irec->rm_blockcount != XFS_AGFL_BLOCK(mp) + 1)
+			goto out_bad_rec;
+	} else {
+		/* check for valid extent range, including overflow */
+		if (!xfs_verify_agbno(mp, agno, irec->rm_startblock))
+			goto out_bad_rec;
+		if (irec->rm_startblock >
+				irec->rm_startblock + irec->rm_blockcount)

fs/xfs/libxfs/xfs_rmap.c +5 -1

+	xfs_warn(mp,
+		"Owner 0x%llx, flags 0x%x, start block 0x%x block count 0x%x",
+		irec->rm_owner, irec->rm_flags, irec->rm_startblock,
+		irec->rm_blockcount);
+	return -EFSCORRUPTED;
 }
 
 struct xfs_find_left_neighbor_info {
--

To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 21 lines · Dave Chinner

fs/xfs/libxfs/xfs_refcount.c +9 -4

@@ -111,16 +111,51 @@ xfs_refcount_get_rec(
 	struct xfs_refcount_irec	*irec,
 	int				*stat)
 {
+	struct xfs_mount		*mp = cur->bc_mp;
+	xfs_agnumber_t			agno = cur->bc_private.a.agno;
 	union xfs_btree_rec		*rec;
 	int				error;
+	xfs_agblock_t			realstart;
 
 	error = xfs_btree_get_rec(cur, &rec, stat);
-	if (!error && *stat == 1) {
-		xfs_refcount_btrec_to_irec(rec, irec);
-		trace_xfs_refcount_get(cur->bc_mp, cur->bc_private.a.agno,
-				irec);
+	if (error || !*stat)
+		return error;
+
+	xfs_refcount_btrec_to_irec(rec, irec);
+
+	agno = cur->bc_private.a.agno;

fs/xfs/libxfs/xfs_refcount.c xfs_refcount_get_rec( · line 136

Quoted 10 lines

fs/xfs/libxfs/xfs_refcount.c +9 -0

+	if (irec->rc_blockcount == 0 || irec->rc_blockcount > MAXREFCEXTLEN)
+		goto out_bad_rec;
+
+	/* handle special COW-staging state */
+	realstart = irec->rc_startblock;
+	if (realstart & XFS_REFC_COW_START) {
+		if (irec->rc_refcount != 1)
+			goto out_bad_rec;
+		realstart &= ~XFS_REFC_COW_START;
 	}

If the record does not have the cow "flag" set then the refcount cannot be 1, so this needs an else clause:

} else {
	if (irec->rc_refcount == 1)
		goto out_bad_rec;
}

fs/xfs/libxfs/xfs_rmap.c xfs_rmap_get_rec( · line 228

Quoted 4 lines

fs/xfs/libxfs/xfs_rmap.c +4 -0

+	if (irec->rm_owner == 0)
+		goto out_bad_rec;
+	if (irec->rm_owner > XFS_MAXINUMBER &&
+	    irec->rm_owner <= XFS_RMAP_OWN_MIN)

rm_owner should be (between XFS_RMAP_OWN_FS and XFS_RMAP_OWN_MIN) or (pass xfs_verify_fsino()) since we cannot have inodes between EOFS and MAXINUMBER.

Review xfs_refcount_get_rec( · line 142

Quoted 3 lines

(quoted diff) +2 -0

+		goto out_bad_rec;
+

Should we check the rmap flags here?

fs/xfs/libxfs/xfs_rmap.c xfs_rmap_get_rec( · line 234

Quoted 4 lines

fs/xfs/libxfs/xfs_rmap.c +4 -0

+	return 0;
+out_bad_rec:
+	xfs_warn(mp,
+		"RMAP BTree record corruption in AG %d detected!", agno);

"RMap" ? Or "Reverse Mapping"?

(Consistent capitalization blah blah...)

From: Dave Chinner <[email protected]>

When looking up the root inode at mount time, we don't actually do any verification to check that the inode is allocated and accounted for correctly in the INOBT. Make the checks on the root inode more robust by making it an untrusted lookup. This forces the inode lookup to use the inode btree to verify the inode is allocated and mapped correctly to disk. This will also have the effect of catching a significant number of AGI/INOBT related corruptions in AG 0 at mount time.

Signed-off-by Dave Chinner <[email protected]>

---
 fs/xfs/xfs_mount.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

fs/xfs/xfs_mount.c +5 -2

index 189fa7b615d3..a3378252baa1 100644
@@ -862,9 +862,12 @@ xfs_mountfs(
 	 * Get and sanity-check the root inode.
 	 * Save the pointer to it in the mount structure.
 	 */
-	error = xfs_iget(mp, NULL, sbp->sb_rootino, 0, XFS_ILOCK_EXCL, &rip);
+	error = xfs_iget(mp, NULL, sbp->sb_rootino, XFS_IGET_UNTRUSTED,
+			 XFS_ILOCK_EXCL, &rip);
 	if (error) {
-		xfs_warn(mp, "failed to read root inode");
+		xfs_warn(mp,
+			"Failed to read root inode 0x%llx, error %d",
+			sbp->sb_rootino, -error);
 		goto out_log_dealloc;
 	}
 

Signature · 6 lines

2.17.0

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Dave Chinner <[email protected]>

When looking up the root inode at mount time, we don't actually do any verification to check that the inode is allocated and accounted for correctly in the INOBT. Make the checks on the root inode more robust by making it an untrusted lookup. This forces the inode lookup to use the inode btree to verify the inode is allocated and mapped correctly to disk. This will also have the effect of catching a significant number of AGI/INOBT related corruptions in AG 0 at mount time.

Signed-off-by Dave Chinner <[email protected]>

---
 fs/xfs/xfs_mount.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

fs/xfs/xfs_mount.c +5 -2

index 189fa7b615d3..a3378252baa1 100644
@@ -862,9 +862,12 @@ xfs_mountfs(
 	 * Get and sanity-check the root inode.
 	 * Save the pointer to it in the mount structure.
 	 */
-	error = xfs_iget(mp, NULL, sbp->sb_rootino, 0, XFS_ILOCK_EXCL, &rip);
+	error = xfs_iget(mp, NULL, sbp->sb_rootino, XFS_IGET_UNTRUSTED,
+			 XFS_ILOCK_EXCL, &rip);
 	if (error) {
-		xfs_warn(mp, "failed to read root inode");
+		xfs_warn(mp,
+			"Failed to read root inode 0x%llx, error %d",
+			sbp->sb_rootino, -error);
 		goto out_log_dealloc;
 	}
 

Signature · 6 lines

2.17.0

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Dave Chinner <[email protected]>

In xfs_imap_to_bp(), we convert a -EFSCORRUPTED error to -EINVAL if we are doing an untrusted lookup. This is done because we need failed filehandle lookups to report -ESTALE to the caller, and it does this by converting -EINVAL and -ENOENT errors to -ESTALE.

The squashing of EFSCORRUPTED in imap_to_bp makes it impossible for for xfs_iget(UNTRUSTED) callers to determine the difference between "inode does not exist" and "corruption detected during lookup". We realy need that distinction in places calling xfS_iget(UNTRUSTED), so move the filehandle error case handling all the way out to xfs_nfs_get_inode() where it is needed.

Signed-off-by Dave Chinner <[email protected]>

---

 fs/xfs/libxfs/xfs_inode_buf.c |  5 -----
 fs/xfs/xfs_export.c           | 15 ++++++++++++---
 2 files changed, 12 insertions(+), 8 deletions(-)

fs/xfs/libxfs/xfs_inode_buf.c +0 -5

index ea64be7cbd98..0a22c39325cd 100644
@@ -189,11 +189,6 @@ xfs_imap_to_bp(
 			ASSERT(buf_flags & XBF_TRYLOCK);
 			return error;
 		}
-
-		if (error == -EFSCORRUPTED &&
-		    (iget_flags & XFS_IGET_UNTRUSTED))
-			return -EINVAL;
-
 		xfs_warn(mp, "%s: xfs_trans_read_buf() returned error %d.",
 			__func__, error);
 		return error;

fs/xfs/xfs_export.c +12 -3

index 7af7d6faa709..3cf4682e2510 100644
@@ -128,15 +128,24 @@ xfs_nfs_get_inode(
 	 */
 	error = xfs_iget(mp, NULL, ino, XFS_IGET_UNTRUSTED, 0, &ip);
 	if (error) {
+
 		/*
 		 * EINVAL means the inode cluster doesn't exist anymore.
-		 * This implies the filehandle is stale, so we should
-		 * translate it here.
+		 * EFSCORRUPTED means the metadata pointing to the inode cluster
+		 * or the inode cluster itself is corrupt.  This implies the
+		 * filehandle is stale, so we should translate it here.
 		 * We don't use ESTALE directly down the chain to not
 		 * confuse applications using bulkstat that expect EINVAL.
 		 */
-		if (error == -EINVAL || error == -ENOENT)
+		switch (error) {
+		case -EINVAL:
+		case -ENOENT:
+		case -EFSCORRUPTED:
 			error = -ESTALE;
+			break;
+		default:
+			break;
+		}
 		return ERR_PTR(error);
 	}
 

Signature · 6 lines

2.17.0

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Dave Chinner <[email protected]>

In xfs_imap_to_bp(), we convert a -EFSCORRUPTED error to -EINVAL if we are doing an untrusted lookup. This is done because we need failed filehandle lookups to report -ESTALE to the caller, and it does this by converting -EINVAL and -ENOENT errors to -ESTALE.

The squashing of EFSCORRUPTED in imap_to_bp makes it impossible for for xfs_iget(UNTRUSTED) callers to determine the difference between "inode does not exist" and "corruption detected during lookup". We realy need that distinction in places calling xfS_iget(UNTRUSTED), so move the filehandle error case handling all the way out to xfs_nfs_get_inode() where it is needed.

Signed-off-by Dave Chinner <[email protected]>

---
 fs/xfs/libxfs/xfs_inode_buf.c |  5 -----
 fs/xfs/xfs_export.c           | 15 ++++++++++++---
 2 files changed, 12 insertions(+), 8 deletions(-)

fs/xfs/libxfs/xfs_inode_buf.c +0 -5

index ea64be7cbd98..0a22c39325cd 100644
@@ -189,11 +189,6 @@ xfs_imap_to_bp(
 			ASSERT(buf_flags & XBF_TRYLOCK);
 			return error;
 		}
-
-		if (error == -EFSCORRUPTED &&
-		    (iget_flags & XFS_IGET_UNTRUSTED))
-			return -EINVAL;
-
 		xfs_warn(mp, "%s: xfs_trans_read_buf() returned error %d.",
 			__func__, error);
 		return error;

fs/xfs/xfs_export.c +12 -3

index 7af7d6faa709..3cf4682e2510 100644
@@ -128,15 +128,24 @@ xfs_nfs_get_inode(
 	 */
 	error = xfs_iget(mp, NULL, ino, XFS_IGET_UNTRUSTED, 0, &ip);
 	if (error) {
+
 		/*
 		 * EINVAL means the inode cluster doesn't exist anymore.
-		 * This implies the filehandle is stale, so we should
-		 * translate it here.
+		 * EFSCORRUPTED means the metadata pointing to the inode cluster
+		 * or the inode cluster itself is corrupt.  This implies the
+		 * filehandle is stale, so we should translate it here.
 		 * We don't use ESTALE directly down the chain to not
 		 * confuse applications using bulkstat that expect EINVAL.
 		 */
-		if (error == -EINVAL || error == -ENOENT)
+		switch (error) {
+		case -EINVAL:
+		case -ENOENT:
+		case -EFSCORRUPTED:
 			error = -ESTALE;
+			break;
+		default:
+			break;
+		}
 		return ERR_PTR(error);
 	}
 

Signature · 6 lines

2.17.0

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html