xfs: handle inode extent count mismatch
cover xfs: handle inode extent count mismatch
Hi folks,
Wen Xu provided an image that caused a crash allocating an extent. The AGFL was corrupt, as was the inode data fork extent count. The combination of the two corruptions could lead to a delalloc extent being allocated on write, and then when allocation fails because the AGFL was corrupt, it would try to punch out the delalloc extent which would then try to convert the format of the extent list in the inode data fork without a transaction. This would crash.
The following two patches address this - the first makes xfs_bunmapi() return EFSCORRUPTED if it tries to change the inode fork format without a transaction context and avoids the crash. THe second makes the inode verifier detect this specific inode fork corruption and prevents any attempt to access it with EFSCORRUPTED.
Comments?
Cheers,
Dave.
PATCH 1/2 xfs: transactionless xfs_bunmapi shouldn't do format conversion
From: Dave Chinner <[email protected]>
If we are punching out a delalloc extent, xfs_bunmapi() does not have a transaction context and should not ever need to convert the on-disk extent format. If such a thing is attempted (e.g. via a corrupt inode extent count in extent format) then we should abort with an EFSCORRUPTED error. Unfortunately, we don't do that and crash instead:
XFS (loop0): page discard on page 0000000005fd24f3, inode 0x75e5, offset 0.
==================================================================
BUG: KASAN: null-ptr-deref in xfs_alloc_get_freelist+0x115/0x350 Read of size 8 at addr 0000000000000028 by task a.out/1406 CPU: 0 PID: 1406 Comm: a.out Not tainted 4.17.0-rc4-kasan #2 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014 Call Trace: dump_stack+0x7b/0xb5 kasan_report+0x10c/0x390 __asan_load8+0x54/0x90 xfs_alloc_get_freelist+0x115/0x350 xfs_alloc_fix_freelist+0x35b/0x830 xfs_alloc_vextent+0x215/0x990 xfs_bmap_extents_to_btree+0x30d/0x940
.....
By returning an error here, we avoid such crashes when punching out a delalloc page because we don't try to fix up an AG freelist without a transaction. Hence we get an error like so:
XFS (loop0): page discard on page ffffea00040ae640, inode 0x75e5, offset 0. XFS (loop0): page discard unable to remove delalloc mapping.
And the filesystem continues to operate and the stale mapping is cleaned up when the inode is reclaimed.
--- fs/xfs/libxfs/xfs_bmap.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
fs/xfs/libxfs/xfs_bmap.c +13 -1
@@ -5458,10 +5458,18 @@ __xfs_bunmapi( *rlen = end - start +1 ;/* -* Convert to a btree if necessary. +* Convert the BMBT root format if necessary. This should only occur in +* transaction contexts and not when removing delalloc extents from +* the in-core extent tree. If we don't have a transaction, then we've +* got some form of corruption somewhere, so return an error +* immediately. */ if (xfs_bmap_needs_btree (ip, whichfork)) {ASSERT (cur ==NULL ); +if (!tp) { + error = -EFSCORRUPTED ; + goto error0; + } error =xfs_bmap_extents_to_btree (tp, ip, firstblock, dfops, &cur,0 , &tmp_logflags, whichfork); logflags |= tmp_logflags;
@@ -5473,6 +5481,10 @@ __xfs_bunmapi(*/ else if (xfs_bmap_wants_extents(ip, whichfork)) {ASSERT (cur !=NULL ); +if (!tp) { + error = -EFSCORRUPTED ; + goto error0; + } error =xfs_bmap_btree_to_extents (tp, ip, cur, &tmp_logflags, whichfork); logflags |= tmp_logflags;
Signature · 1 line
2.17.0
Re: [PATCH 1/2] xfs: transactionless xfs_bunmapi shouldn't do format conversion
Quoted 23 lines · Dave Chinner
From: Dave Chinner <[email protected]>
If we are punching out a delalloc extent,
xfs_bunmapi()does not have a transaction context and should not ever need to convert the on-disk extent format. If such a thing is attempted (e.g. via a corrupt inode extent count in extent format) then we should abort with an EFSCORRUPTED error. Unfortunately, we don't do that and crash instead:XFS (loop0): page discard on page
0000000005fd24f3, inode0x75e5, offset 0.
==================================================================BUG: KASAN: null-ptr-deref in xfs_alloc_get_freelist+0x115/0x350 Read of size 8 at addr 0000000000000028 by task a.out/1406 CPU: 0 PID: 1406 Comm: a.out Not tainted 4.17.0-rc4-kasan #2 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014 Call Trace: dump_stack+0x7b/0xb5 kasan_report+0x10c/0x390 __asan_load8+0x54/0x90 xfs_alloc_get_freelist+0x115/0x350 xfs_alloc_fix_freelist+0x35b/0x830 xfs_alloc_vextent+0x215/0x990 xfs_bmap_extents_to_btree+0x30d/0x940
Quoted 5 lines
.....
By returning an error here, we avoid such crashes when punching out a delalloc page because we don't try to fix up an AG freelist without a transaction. Hence we get an error like so:
Um, isn't erroring out here leaving a dirty bomb in the in-core metadata? Like you say:
Quoted 2 lines
XFS (loop0): page discard on page
ffffea00040ae640, inode0x75e5, offset 0. XFS (loop0): page discard unable to remove delalloc mapping.
We know the fs is corrupt, we might as well shut down now rather than let this burp out later.
I get that people don't want to touch well seasoned code, but xfs_bunmapi is this big unwieldly function that's crying out for a refactor. It's 330 lines long and can be called from various contexts (data/attr fork, punch delalloc, etc.)...
...it's also weird that xfs_bmap_punch_delalloc_range calls xfs_bunmapi with no transaction and a xfs_defer that we dump on the ground.
So yes, I think the patch does fix the crash, but it's kinda gross.
Thoughts?
--D
Quoted 51 lines
And the filesystem continues to operate and the stale mapping is cleaned up when the inode is reclaimed.
Reported-by Wen Xu <[email protected]>Signed-off-by Dave Chinner <[email protected]>--- fs/xfs/libxfs/xfs_bmap.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)fs/xfs/libxfs/xfs_bmap.c +13
-1@@ -5458,10 +5458,18 @@ __xfs_bunmapi( *rlen = end - start +1 ;/* -* Convert to a btree if necessary. +* Convert the BMBT root format if necessary. This should only occur in +* transaction contexts and not when removing delalloc extents from +* the in-core extent tree. If we don't have a transaction, then we've +* got some form of corruption somewhere, so return an error +* immediately. */ if (xfs_bmap_needs_btree (ip, whichfork)) {ASSERT (cur ==NULL ); +if (!tp) { + error = -EFSCORRUPTED ; + goto error0; + } error =xfs_bmap_extents_to_btree (tp, ip, firstblock, dfops, &cur,0 , &tmp_logflags, whichfork); logflags |= tmp_logflags;@@ -5473,6 +5481,10 @@ __xfs_bunmapi(*/ else if (xfs_bmap_wants_extents(ip, whichfork)) {ASSERT (cur !=NULL ); +if (!tp) { + error = -EFSCORRUPTED ; + goto error0; + } error =xfs_bmap_btree_to_extents (tp, ip, cur, &tmp_logflags, whichfork); logflags |= tmp_logflags;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
Re: [PATCH 1/2] xfs: transactionless xfs_bunmapi shouldn't do format conversion
Quoted 26 lines · Darrick J. Wong
Quoted 25 lines · Dave Chinner
From: Dave Chinner <[email protected]>
If we are punching out a delalloc extent,
xfs_bunmapi()does not have a transaction context and should not ever need to convert the on-disk extent format. If such a thing is attempted (e.g. via a corrupt inode extent count in extent format) then we should abort with an EFSCORRUPTED error. Unfortunately, we don't do that and crash instead:XFS (loop0): page discard on page
0000000005fd24f3, inode0x75e5, offset 0.
==================================================================BUG: KASAN: null-ptr-deref in xfs_alloc_get_freelist+0x115/0x350 Read of size 8 at addr 0000000000000028 by task a.out/1406 CPU: 0 PID: 1406 Comm: a.out Not tainted 4.17.0-rc4-kasan #2 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014 Call Trace: dump_stack+0x7b/0xb5 kasan_report+0x10c/0x390 __asan_load8+0x54/0x90 xfs_alloc_get_freelist+0x115/0x350 xfs_alloc_fix_freelist+0x35b/0x830 xfs_alloc_vextent+0x215/0x990 xfs_bmap_extents_to_btree+0x30d/0x940.....
Quoted 5 lines
Quoted 3 lines
By returning an error here, we avoid such crashes when punching out a delalloc page because we don't try to fix up an AG freelist without a transaction. Hence we get an error like so:
Um, isn't erroring out here leaving a dirty bomb in the in-core metadata?
Not that I can tell. We've already trashed the dirty page state by this point, so the page cache can safely reclaim the page and the delalloc range over it will never get written. And the XFS inode cleanup code didn't have any issues with the way the error was handled, either, because the delalloc range was actually removed before the fork format error was triggered.
IOWs, there is no dirty, stale page state or delalloc extents hanging around if this error fires.
Quoted 7 lines
Like you say:
Quoted 2 lines
XFS (loop0): page discard on page
ffffea00040ae640, inode0x75e5, offset 0. XFS (loop0): page discard unable to remove delalloc mapping.We know the fs is corrupt, we might as well shut down now rather than let this burp out later.
xfs_bunmapi() doesn't do shutdowns - the higher level code does a shutdown on error if it is necessary, otherwise it just propagates the error. In this case it has cleaned up correctly, propagates the error and it gets back to userspace on the next fsync, and we're fine to continue onwards as there was no unrecoverable error....
Quoted 7 lines
I get that people don't want to touch well seasoned code, but
xfs_bunmapiis this big unwieldly function that's crying out for a refactor. It's 330 lines long and can be called from various contexts (data/attr fork, punch delalloc, etc.)......it's also weird that
xfs_bmap_punch_delalloc_rangecallsxfs_bunmapiwith no transaction and axfs_deferthat we dump on the ground.
Yes, and yes.
Quoted 1 line
So yes, I think the patch does fix the crash, but it's kinda gross.
Yes, it is.
But OTOH, I don't want to risk a bunch of filesystem corrupting regressions across the entire XFS userbase just to fix a trivially simple crash that requires an extremely unlikely co-ordinated corruption of an inode data fork and an AGFL, and to simultaneously have ENOSPC in every other AGF in the filesystem.
Put "refactor xfs_bunmapi()" on the list of "things to do when there's nothing else to do"...
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]
Re: [PATCH 1/2] xfs: transactionless xfs_bunmapi shouldn't do format conversion
Quoted 40 lines · Dave Chinner
Quoted 26 lines · Darrick J. Wong
Quoted 25 lines · Dave Chinner
From: Dave Chinner <[email protected]>
If we are punching out a delalloc extent,
xfs_bunmapi()does not have a transaction context and should not ever need to convert the on-disk extent format. If such a thing is attempted (e.g. via a corrupt inode extent count in extent format) then we should abort with an EFSCORRUPTED error. Unfortunately, we don't do that and crash instead:XFS (loop0): page discard on page
0000000005fd24f3, inode0x75e5, offset 0.
==================================================================BUG: KASAN: null-ptr-deref in xfs_alloc_get_freelist+0x115/0x350 Read of size 8 at addr 0000000000000028 by task a.out/1406 CPU: 0 PID: 1406 Comm: a.out Not tainted 4.17.0-rc4-kasan #2 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014 Call Trace: dump_stack+0x7b/0xb5 kasan_report+0x10c/0x390 __asan_load8+0x54/0x90 xfs_alloc_get_freelist+0x115/0x350 xfs_alloc_fix_freelist+0x35b/0x830 xfs_alloc_vextent+0x215/0x990 xfs_bmap_extents_to_btree+0x30d/0x940.....
Quoted 5 lines
Quoted 3 lines
By returning an error here, we avoid such crashes when punching out a delalloc page because we don't try to fix up an AG freelist without a transaction. Hence we get an error like so:
Um, isn't erroring out here leaving a dirty bomb in the in-core metadata?
Not that I can tell. We've already trashed the dirty page state by this point, so the page cache can safely reclaim the page and the delalloc range over it will never get written. And the XFS inode cleanup code didn't have any issues with the way the error was handled, either, because the delalloc range was actually removed before the fork format error was triggered.
Quoted 2 lines
IOWs, there is no dirty, stale page state or delalloc extents hanging around if this error fires.
Hmmm, well I guess I'll pull this one in and look for problems.
I wonder, is there a <cough> testcase for this? Or a fuzz-o-matic to turn all these things into regression tests?
(Yeah, I know there won't be one for syzbot, I dug through its code and had to reset my brain by reading mballoc.c. :P)
Quoted 5 lines
Quoted 5 lines
Like you say:
Quoted 2 lines
XFS (loop0): page discard on page
ffffea00040ae640, inode0x75e5, offset 0. XFS (loop0): page discard unable to remove delalloc mapping.
Quoted 8 lines
Quoted 2 lines
We know the fs is corrupt, we might as well shut down now rather than let this burp out later.
xfs_bunmapi()doesn't do shutdowns - the higher level code does a shutdown on error if it is necessary, otherwise it just propagates the error. In this case it has cleaned up correctly, propagates the error and it gets back to userspace on the next fsync, and we're fine to continue onwards as there was no unrecoverable error....
Fair enough.
Quoted 14 lines
Quoted 7 lines
I get that people don't want to touch well seasoned code, but
xfs_bunmapiis this big unwieldly function that's crying out for a refactor. It's 330 lines long and can be called from various contexts (data/attr fork, punch delalloc, etc.)......it's also weird that
xfs_bmap_punch_delalloc_rangecallsxfs_bunmapiwith no transaction and axfs_deferthat we dump on the ground.Yes, and yes.
Quoted 1 line
So yes, I think the patch does fix the crash, but it's kinda gross.
Yes, it is.
Quoted 8 lines
But OTOH, I don't want to risk a bunch of filesystem corrupting regressions across the entire XFS userbase just to fix a trivially simple crash that requires an extremely unlikely co-ordinated corruption of an inode data fork and an AGFL, and to simultaneously have ENOSPC in every other AGF in the filesystem.
Put "refactor
xfs_bunmapi()" on the list of "things to do when there's nothing else to do"...
So in 2066 after the polar ice caps melt after the XFS LOGHAMMER attack has finally been put down? Ok. :)
(But no, seriously, if anyone's looking for a little refactoring + domain knowledge enhancement of the bmapi code...)
--D
Quoted 10 lines
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
Re: [PATCH 1/2] xfs: transactionless xfs_bunmapi shouldn't do format conversion
Quoted 41 lines · Darrick J. Wong
Quoted 40 lines · Dave Chinner
Quoted 26 lines · Darrick J. Wong
Quoted 25 lines · Dave Chinner
From: Dave Chinner <[email protected]>
If we are punching out a delalloc extent,
xfs_bunmapi()does not have a transaction context and should not ever need to convert the on-disk extent format. If such a thing is attempted (e.g. via a corrupt inode extent count in extent format) then we should abort with an EFSCORRUPTED error. Unfortunately, we don't do that and crash instead:XFS (loop0): page discard on page
0000000005fd24f3, inode0x75e5, offset 0.
==================================================================BUG: KASAN: null-ptr-deref in xfs_alloc_get_freelist+0x115/0x350 Read of size 8 at addr 0000000000000028 by task a.out/1406 CPU: 0 PID: 1406 Comm: a.out Not tainted 4.17.0-rc4-kasan #2 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014 Call Trace: dump_stack+0x7b/0xb5 kasan_report+0x10c/0x390 __asan_load8+0x54/0x90 xfs_alloc_get_freelist+0x115/0x350 xfs_alloc_fix_freelist+0x35b/0x830 xfs_alloc_vextent+0x215/0x990 xfs_bmap_extents_to_btree+0x30d/0x940.....
Quoted 5 lines
Quoted 3 lines
By returning an error here, we avoid such crashes when punching out a delalloc page because we don't try to fix up an AG freelist without a transaction. Hence we get an error like so:
Um, isn't erroring out here leaving a dirty bomb in the in-core metadata?
Not that I can tell. We've already trashed the dirty page state by this point, so the page cache can safely reclaim the page and the delalloc range over it will never get written. And the XFS inode cleanup code didn't have any issues with the way the error was handled, either, because the delalloc range was actually removed before the fork format error was triggered.
Quoted 7 lines
Quoted 2 lines
IOWs, there is no dirty, stale page state or delalloc extents hanging around if this error fires.
Hmmm, well I guess I'll pull this one in and look for problems.
I wonder, is there a <cough> testcase for this? Or a fuzz-o-matic to turn all these things into regression tests?
No test case. Should be able to create one easily enough with xfs_db, though I haven't tried. Do the inode fuzzer tests screw with the extent count?
Quoted 11 lines
Quoted 8 lines
But OTOH, I don't want to risk a bunch of filesystem corrupting regressions across the entire XFS userbase just to fix a trivially simple crash that requires an extremely unlikely co-ordinated corruption of an inode data fork and an AGFL, and to simultaneously have ENOSPC in every other AGF in the filesystem.
Put "refactor
xfs_bunmapi()" on the list of "things to do when there's nothing else to do"...So in 2066 after the polar ice caps melt after the XFS LOGHAMMER attack has finally been put down? Ok. :)
I'm sure someone will have reason to factor it before then :P
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]
Re: [PATCH 1/2] xfs: transactionless xfs_bunmapi shouldn't do format conversion
Quoted 45 lines · Dave Chinner
Quoted 44 lines · Darrick J. Wong
Quoted 42 lines · Dave Chinner
Quoted 26 lines · Darrick J. Wong
Quoted 25 lines · Dave Chinner
From: Dave Chinner <[email protected]>
If we are punching out a delalloc extent,
xfs_bunmapi()does not have a transaction context and should not ever need to convert the on-disk extent format. If such a thing is attempted (e.g. via a corrupt inode extent count in extent format) then we should abort with an EFSCORRUPTED error. Unfortunately, we don't do that and crash instead:XFS (loop0): page discard on page
0000000005fd24f3, inode0x75e5, offset 0.
==================================================================BUG: KASAN: null-ptr-deref in xfs_alloc_get_freelist+0x115/0x350 Read of size 8 at addr 0000000000000028 by task a.out/1406 CPU: 0 PID: 1406 Comm: a.out Not tainted 4.17.0-rc4-kasan #2 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014 Call Trace: dump_stack+0x7b/0xb5 kasan_report+0x10c/0x390 __asan_load8+0x54/0x90 xfs_alloc_get_freelist+0x115/0x350 xfs_alloc_fix_freelist+0x35b/0x830 xfs_alloc_vextent+0x215/0x990 xfs_bmap_extents_to_btree+0x30d/0x940.....
Quoted 5 lines
Quoted 3 lines
By returning an error here, we avoid such crashes when punching out a delalloc page because we don't try to fix up an AG freelist without a transaction. Hence we get an error like so:
Um, isn't erroring out here leaving a dirty bomb in the in-core metadata?
Not that I can tell. We've already trashed the dirty page state by this point, so the page cache can safely reclaim the page and the delalloc range over it will never get written. And the XFS inode cleanup code didn't have any issues with the way the error was handled, either, because the delalloc range was actually removed before the fork format error was triggered.
IOWs, there is no dirty, stale page state or delalloc extents hanging around if this error fires.
Quoted 8 lines
Quoted 4 lines
Hmmm, well I guess I'll pull this one in and look for problems.
I wonder, is there a <cough> testcase for this? Or a fuzz-o-matic to turn all these things into regression tests?
No test case. Should be able to create one easily enough with
xfs_db, though I haven't tried. Do the inode fuzzer tests screw with the extent count?
The existing set of fuzz tests won't catch this because they go straight into repair attempts to see if scrub/repair will deal with bad nextents. They don't try to modify the corrupted fs.
They also do it slowly because fuzzing nextents is simply a part of fuzzing every field in a extents-format file inode, and I suspect that we don't really want to make fuzz testing a regular part of xfstests because that immediately triples the auto group runtime. :)
So, targeted test please? :)
I will also work on a fuzz series that skips scrub/repair and goes straight to writing to the corrupted fs to see what happens.
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
But OTOH, I don't want to risk a bunch of filesystem corrupting regressions across the entire XFS userbase just to fix a trivially simple crash that requires an extremely unlikely co-ordinated corruption of an inode data fork and an AGFL, and to simultaneously have ENOSPC in every other AGF in the filesystem.
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
Put "refactor
xfs_bunmapi()" on the list of "things to do when there's nothing else to do"...So in 2066 after the polar ice caps melt after the XFS LOGHAMMER attack has finally been put down? Ok. :)
I'm sure someone will have reason to factor it before then :P
I ... forgot that hch already did. :/
--D
Quoted 10 lines
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
Re: [PATCH 1/2] xfs: transactionless xfs_bunmapi shouldn't do format conversion
In reply to Darrick J. WongQuoted 6 lines · Darrick J. Wong
...it's also weird that
xfs_bmap_punch_delalloc_rangecallsxfs_bunmapiwith no transaction and axfs_deferthat we dump on the ground.So yes, I think the patch does fix the crash, but it's kinda gross.
Thoughts?
I've got an alternative solution:
http://git.infradead.org/users/hch/xfs.git/commitdiff/a1c0685b2085b448cbe02f0f9ff0c8771e3f4496
The only bit that is missing is removing the now unused support for a NULL tp in __xfs_bunmapi..
Re: [PATCH 1/2] xfs: transactionless xfs_bunmapi shouldn't do format conversion
Quoted 6 lines · Christoph Hellwig
Quoted 5 lines · Darrick J. Wong
...it's also weird that
xfs_bmap_punch_delalloc_rangecallsxfs_bunmapiwith no transaction and axfs_deferthat we dump on the ground.So yes, I think the patch does fix the crash, but it's kinda gross.
Quoted 8 lines
Quoted 1 line
Thoughts?
I've got an alternative solution:
http://git.infradead.org/users/hch/xfs.git/commitdiff/a1c0685b2085b448cbe02f0f9ff0c8771e3f4496
The only bit that is missing is removing the now unused support for a NULL tp in
__xfs_bunmapi..
Ah, I forgot about that patch. Thanks for the reminder, Christoph!
Darrick, can we get Christoph's patch in as a standalone bug fix rather than wait for the bufferhead removal to be merged?
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]
Re: [PATCH 1/2] xfs: transactionless xfs_bunmapi shouldn't do format conversion
Quoted 13 lines · Dave Chinner
Quoted 12 lines · Christoph Hellwig
Quoted 6 lines · Darrick J. Wong
...it's also weird that
xfs_bmap_punch_delalloc_rangecallsxfs_bunmapiwith no transaction and axfs_deferthat we dump on the ground.So yes, I think the patch does fix the crash, but it's kinda gross.
Thoughts?
I've got an alternative solution:
http://git.infradead.org/users/hch/xfs.git/commitdiff/a1c0685b2085b448cbe02f0f9ff0c8771e3f4496
Quoted 7 lines
Quoted 2 lines
The only bit that is missing is removing the now unused support for a NULL tp in
__xfs_bunmapi..Ah, I forgot about that patch. Thanks for the reminder, Christoph!
Darrick, can we get Christoph's patch in as a standalone bug fix rather than wait for the bufferhead removal to be merged?
Ok, will do. AFAICT if I merge that patch then I can drop this one, right?
--D
Quoted 10 lines
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
Re: [PATCH 1/2] xfs: transactionless xfs_bunmapi shouldn't do format conversion
Quoted 17 lines · Darrick J. Wong
Quoted 16 lines · Dave Chinner
Quoted 14 lines · Christoph Hellwig
Quoted 6 lines · Darrick J. Wong
...it's also weird that
xfs_bmap_punch_delalloc_rangecallsxfs_bunmapiwith no transaction and axfs_deferthat we dump on the ground.So yes, I think the patch does fix the crash, but it's kinda gross.
Thoughts?
I've got an alternative solution:
http://git.infradead.org/users/hch/xfs.git/commitdiff/a1c0685b2085b448cbe02f0f9ff0c8771e3f4496
The only bit that is missing is removing the now unused support for a NULL tp in
__xfs_bunmapi..
Quoted 7 lines
Quoted 4 lines
Ah, I forgot about that patch. Thanks for the reminder, Christoph!
Darrick, can we get Christoph's patch in as a standalone bug fix rather than wait for the bufferhead removal to be merged?
Ok, will do. AFAICT if I merge that patch then I can drop this one, right?
Yup, should be able to, as this is the only vector into the problem code.
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]
PATCH 2/2 xfs: More robust inode extent count validation
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
When the inode is in extent format, it can't have more extents that fit in the inode fork. We don't currenty check this, and so this corruption goes unnoticed by the inode verifiers. This can lead to crashes operating on invalid in-memory structures.
Attempts to access such a inode will now error out in the verifier rather than allowing modification operations to proceed.
--- fs/xfs/libxfs/xfs_format.h | 3 ++ fs/xfs/libxfs/xfs_inode_buf.c | 74 +++++++++++++++++++++-------------- 2 files changed, 48 insertions(+), 29 deletions(-)
fs/xfs/libxfs/xfs_format.h +3 -0
@@ -962,6 +962,9 @@ typedef enum xfs_dinode_fmt { XFS_DFORK_DSIZE(dip, mp) : \ XFS_DFORK_ASIZE(dip, mp)) +#define XFS_DFORK_MAXEXT (dip, mp, w) \ + (XFS_DFORK_SIZE(dip, mp, w) / sizeof(xfs_bmbt_rec_t)) +/* * Return pointers to the data or attribute forks. */
fs/xfs/libxfs/xfs_inode_buf.c +45 -29
92 lines not on this page · raw message
Signature · 1 line
2.17.0
Re: [PATCH 2/2] xfs: More robust inode extent count validation
Quoted 52 lines · Dave Chinner
From: Dave Chinner <[email protected]>
When the inode is in extent format, it can't have more extents that fit in the inode fork. We don't currenty check this, and so this corruption goes unnoticed by the inode verifiers. This can lead to crashes operating on invalid in-memory structures.
Attempts to access such a inode will now error out in the verifier rather than allowing modification operations to proceed.
Reported-by Wen Xu <[email protected]>Signed-off-by Dave Chinner <[email protected]>--- fs/xfs/libxfs/xfs_format.h | 3 ++ fs/xfs/libxfs/xfs_inode_buf.c | 74 +++++++++++++++++++++-------------- 2 files changed, 48 insertions(+), 29 deletions(-)fs/xfs/libxfs/xfs_format.h +3
-0@@ -962,6 +962,9 @@ typedef enum xfs_dinode_fmt { XFS_DFORK_DSIZE(dip, mp) : \ XFS_DFORK_ASIZE(dip, mp)) +#define XFS_DFORK_MAXEXT (dip, mp, w) \ + (XFS_DFORK_SIZE(dip, mp, w) / sizeof(xfs_bmbt_rec_t)) +/* * Return pointers to the data or attribute forks. */ fs/xfs/libxfs/xfs_inode_buf.c +13
-0@@ -374,6 +374,45 @@ xfs_log_dinode_to_disk( } } +static xfs_failaddr_t +xfs_dinode_verify_fork ( +struct xfs_dinode *dip, +struct xfs_mount *mp, +int whichfork) +{ +uint32_t di_nextents =XFS_DFORK_NEXTENTS (dip, whichfork); + +switch (XFS_DFORK_FORMAT (dip, whichfork)) { +case XFS_DINODE_FMT_LOCAL : +/* +* no local regular files yet +*/
fs/xfs/libxfs/xfs_inode_buf.c xfs_log_dinode_to_disk( · line 399
Quoted 10 lines
fs/xfs/libxfs/xfs_inode_buf.c +10
-0+if (whichfork ==XFS_DATA_FORK ) { +if (S_ISREG (be16_to_cpu (dip->di_mode))) +return __this_address; +if (be64_to_cpu (dip->di_size) > +XFS_DFORK_SIZE (dip, mp, whichfork)) +return __this_address; + } +if (di_nextents) +return __this_address; +/* fall through */
We could break here too, right? There's no point in further checks of di_nextents for local format forks.
fs/xfs/libxfs/xfs_inode_buf.c xfs_log_dinode_to_disk( · line 402
Quoted 3 lines
fs/xfs/libxfs/xfs_inode_buf.c +3
-0+case XFS_DINODE_FMT_EXTENTS : +if (di_nextents >XFS_DFORK_MAXEXT (dip, mp, whichfork)) +return __this_address;
Are we supposed to break here?
--D
Quoted 72 lines
fs/xfs/libxfs/xfs_inode_buf.c +19
-29+case XFS_DINODE_FMT_BTREE : +if (whichfork ==XFS_ATTR_FORK ) +if (di_nextents >MAXAEXTNUM ) +return __this_address; +else if (di_nextents >MAXEXTNUM ) +return __this_address; +break ; +default : +return __this_address; + } +return NULL ; +} + xfs_failaddr_t xfs_dinode_verify( struct xfs_mount *mp,@@ -441,24 +480,9 @@ xfs_dinode_verify(case S_IFREG :case S_IFLNK :case S_IFDIR : -switch (dip->di_format) { -case XFS_DINODE_FMT_LOCAL : -/* -* no local regular files yet -*/ -if (S_ISREG (mode)) -return __this_address; -if (di_size >XFS_DFORK_DSIZE (dip, mp)) -return __this_address; -if (dip->di_nextents) -return __this_address; -/* fall through */ -case XFS_DINODE_FMT_EXTENTS : -case XFS_DINODE_FMT_BTREE : -break ; -default : -return __this_address; - } + fa =xfs_dinode_verify_fork (dip, mp,XFS_DATA_FORK ); +if (fa) +return fa;break ;case 0 :/* Uninitialized inode ok. */ @@ -468,17 +492,9 @@ xfs_dinode_verify( }if (XFS_DFORK_Q (dip)) { -switch (dip->di_aformat) { -case XFS_DINODE_FMT_LOCAL : -if (dip->di_anextents) -return __this_address; -/* fall through */ -case XFS_DINODE_FMT_EXTENTS : -case XFS_DINODE_FMT_BTREE : -break ; -default : -return __this_address; - } + fa =xfs_dinode_verify_fork (dip, mp,XFS_ATTR_FORK ); +if (fa) +return fa; }else {/* * If there is no fork offset, this may be a freshly-made inode 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
Re: [PATCH 2/2] xfs: More robust inode extent count validation
Quoted 67 lines · Darrick J. Wong
Quoted 52 lines · Dave Chinner
From: Dave Chinner <[email protected]>
When the inode is in extent format, it can't have more extents that fit in the inode fork. We don't currenty check this, and so this corruption goes unnoticed by the inode verifiers. This can lead to crashes operating on invalid in-memory structures.
Attempts to access such a inode will now error out in the verifier rather than allowing modification operations to proceed.
Reported-by Wen Xu <[email protected]>Signed-off-by Dave Chinner <[email protected]>--- fs/xfs/libxfs/xfs_format.h | 3 ++ fs/xfs/libxfs/xfs_inode_buf.c | 74 +++++++++++++++++++++-------------- 2 files changed, 48 insertions(+), 29 deletions(-)fs/xfs/libxfs/xfs_format.h +3
-0@@ -962,6 +962,9 @@ typedef enum xfs_dinode_fmt { XFS_DFORK_DSIZE(dip, mp) : \ XFS_DFORK_ASIZE(dip, mp)) +#define XFS_DFORK_MAXEXT (dip, mp, w) \ + (XFS_DFORK_SIZE(dip, mp, w) / sizeof(xfs_bmbt_rec_t)) +/* * Return pointers to the data or attribute forks. */ fs/xfs/libxfs/xfs_inode_buf.c +13
-0@@ -374,6 +374,45 @@ xfs_log_dinode_to_disk( } } +static xfs_failaddr_t +xfs_dinode_verify_fork ( +struct xfs_dinode *dip, +struct xfs_mount *mp, +int whichfork) +{ +uint32_t di_nextents =XFS_DFORK_NEXTENTS (dip, whichfork); + +switch (XFS_DFORK_FORMAT (dip, whichfork)) { +case XFS_DINODE_FMT_LOCAL : +/* +* no local regular files yet +*/ fs/xfs/libxfs/xfs_inode_buf.c xfs_log_dinode_to_disk( · line 399
Quoted 10 lines
fs/xfs/libxfs/xfs_inode_buf.c +10
-0+if (whichfork ==XFS_DATA_FORK ) { +if (S_ISREG (be16_to_cpu (dip->di_mode))) +return __this_address; +if (be64_to_cpu (dip->di_size) > +XFS_DFORK_SIZE (dip, mp, whichfork)) +return __this_address; + } +if (di_nextents) +return __this_address; +/* fall through */ We could break here too, right? There's no point in further checks of
di_nextentsfor local format forks.
Quoted 5 lines
fs/xfs/libxfs/xfs_inode_buf.c xfs_log_dinode_to_disk( · line 402
Quoted 3 lines
fs/xfs/libxfs/xfs_inode_buf.c +3
-0+case XFS_DINODE_FMT_EXTENTS : +if (di_nextents >XFS_DFORK_MAXEXT (dip, mp, whichfork)) +return __this_address;Are we supposed to break here?
They all fall through like they used to, but they could break, too. The behaviour will be the same now.
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]
Darrick J. Wong
Reviewed-byRe: [PATCH 2/2] xfs: More robust inode extent count validation
Quoted 68 lines · Dave Chinner
Quoted 67 lines · Darrick J. Wong
Quoted 52 lines · Dave Chinner
From: Dave Chinner <[email protected]>
When the inode is in extent format, it can't have more extents that fit in the inode fork. We don't currenty check this, and so this corruption goes unnoticed by the inode verifiers. This can lead to crashes operating on invalid in-memory structures.
Attempts to access such a inode will now error out in the verifier rather than allowing modification operations to proceed.
Reported-by Wen Xu <[email protected]>Signed-off-by Dave Chinner <[email protected]>--- fs/xfs/libxfs/xfs_format.h | 3 ++ fs/xfs/libxfs/xfs_inode_buf.c | 74 +++++++++++++++++++++-------------- 2 files changed, 48 insertions(+), 29 deletions(-)fs/xfs/libxfs/xfs_format.h +3
-0@@ -962,6 +962,9 @@ typedef enum xfs_dinode_fmt { XFS_DFORK_DSIZE(dip, mp) : \ XFS_DFORK_ASIZE(dip, mp)) +#define XFS_DFORK_MAXEXT (dip, mp, w) \ + (XFS_DFORK_SIZE(dip, mp, w) / sizeof(xfs_bmbt_rec_t)) +/* * Return pointers to the data or attribute forks. */ fs/xfs/libxfs/xfs_inode_buf.c +13
-0@@ -374,6 +374,45 @@ xfs_log_dinode_to_disk( } } +static xfs_failaddr_t +xfs_dinode_verify_fork ( +struct xfs_dinode *dip, +struct xfs_mount *mp, +int whichfork) +{ +uint32_t di_nextents =XFS_DFORK_NEXTENTS (dip, whichfork); + +switch (XFS_DFORK_FORMAT (dip, whichfork)) { +case XFS_DINODE_FMT_LOCAL : +/* +* no local regular files yet +*/ fs/xfs/libxfs/xfs_inode_buf.c xfs_log_dinode_to_disk( · line 399
Quoted 10 lines
fs/xfs/libxfs/xfs_inode_buf.c +10
-0+if (whichfork ==XFS_DATA_FORK ) { +if (S_ISREG (be16_to_cpu (dip->di_mode))) +return __this_address; +if (be64_to_cpu (dip->di_size) > +XFS_DFORK_SIZE (dip, mp, whichfork)) +return __this_address; + } +if (di_nextents) +return __this_address; +/* fall through */ We could break here too, right? There's no point in further checks of
di_nextentsfor local format forks.
Quoted 8 lines
Quoted 5 lines
fs/xfs/libxfs/xfs_inode_buf.c xfs_log_dinode_to_disk( · line 402
Quoted 3 lines
fs/xfs/libxfs/xfs_inode_buf.c +3
-0+case XFS_DINODE_FMT_EXTENTS : +if (di_nextents >XFS_DFORK_MAXEXT (dip, mp, whichfork)) +return __this_address;Are we supposed to break here?
They all fall through like they used to, but they could break, too. The behaviour will be the same now.
Fair enough.
--D
Quoted 10 lines
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
Re: [PATCH 2/2] xfs: More robust inode extent count validation
In reply to Dave ChinnerQuoted 22 lines · Dave Chinner
From: Dave Chinner <[email protected]>
When the inode is in extent format, it can't have more extents that fit in the inode fork. We don't currenty check this, and so this corruption goes unnoticed by the inode verifiers. This can lead to crashes operating on invalid in-memory structures.
Attempts to access such a inode will now error out in the verifier rather than allowing modification operations to proceed.
Reported-by Wen Xu <[email protected]>Signed-off-by Dave Chinner <[email protected]>--- fs/xfs/libxfs/xfs_format.h | 3 ++ fs/xfs/libxfs/xfs_inode_buf.c | 74 +++++++++++++++++++++-------------- 2 files changed, 48 insertions(+), 29 deletions(-)fs/xfs/libxfs/xfs_format.h +0
-0
fs/xfs/libxfs/xfs_format.h typedef enum xfs_dinode_fmt { · line 966
Quoted 5 lines
fs/xfs/libxfs/xfs_format.h +2
-0@@ -962,6 +962,9 @@ typedef enum xfs_dinode_fmt { XFS_DFORK_DSIZE(dip, mp) : \ XFS_DFORK_ASIZE(dip, mp)) +#define XFS_DFORK_MAXEXT (dip, mp, w) \ + (XFS_DFORK_SIZE(dip, mp, w) / sizeof(xfs_bmbt_rec_t))
struct xfs_bmbt_rec, please.
Also do we really need this macro instead of just open coding it?
fs/xfs/libxfs/xfs_format.h typedef enum xfs_dinode_fmt { · line 969
Quoted 3 lines
fs/xfs/libxfs/xfs_format.h +3
-0+if (di_nextents) +return __this_address; +/* fall through */
seems weird to fall through when the next check is just for di_nextents again. I'd rather break out of the switch and have the common validation after it.
But the basic of the patch look fine to me.