* Re: [Bug 217030] New: [Syzkaller & bisect] There is "xfs_bmapi_convert_delalloc" WARNING in v6.2-rc7 kernel
2023-02-13 10:57 [Bug 217030] New: [Syzkaller & bisect] There is "xfs_bmapi_convert_delalloc" WARNING in v6.2-rc7 kernel bugzilla-daemon
@ 2023-02-13 23:01 ` Dave Chinner
2023-02-13 23:01 ` [Bug 217030] " bugzilla-daemon
2023-02-14 1:23 ` bugzilla-daemon
2 siblings, 0 replies; 4+ messages in thread
From: Dave Chinner @ 2023-02-13 23:01 UTC (permalink / raw)
To: bugzilla-daemon; +Cc: linux-xfs
On Mon, Feb 13, 2023 at 10:57:31AM +0000, bugzilla-daemon@kernel.org wrote:
> Bisected and found the first bad commit is:
> 7348b322332d8602a4133f0b861334ea021b134a
> xfs: xfs_bmap_punch_delalloc_range() should take a byte range
I don't see how that commit makes any material difference to the
code; the ranges being punched are the same, just being passed in
different units.
The test filesystem image has a 1kB block size, and it has a bad CRC
in the root bnobt block, hence any attempt to allocate fails
immediately with a corrupt root btree block. The first write done
by the reproducer is an O_SYNC write, so it writes 0x1400 bytes into
the page cache (5 1kB blocks) and then internally the write() runs
fsync() which then fails to allocate the blocks in the writeback
path.
This writeback failure then calls ->discard_page() on the write range,
which punches out the delalloc block underneath the failed write.
At the same time, the other thread is doing a direct IO write, and
it sees there are dirty page cache pages over the page cache and it
attempts to writeback the page. It calls into ->map_blocks which
then sees a delalloc extent and tries to convert it. By the time
writeback has locked the folio, locked the ILOCK and runs the
delalloc extent conversion, the delalloc extent has been punched,
and it fails to find the delalloc extent to convert. It then issues
the WARN_ON() reported in this bug report.
I don't think this a failure that is a result of the commit that the
bisect identified - I think the race condition has been there for a
lot longer, and for some reason we've perturbed the code enough
to now expose the race condition occasionally. i.e. the
->discard_folio path is not invalidating the folio or marking it
with an error when we punch the backing store from under it, so it's
remaining dirty in the page cache but without any backing store for
writeback.
Hence I suspect the problem here is that we aren't calling
folio_set_error() on folios that we fail the first delalloc block
conversion on (i.e. count is zero) so this isn't operating like an
IO error. i.e. page_endio() calls folio_set_error() when a
writeback IO fails, and we are not doing that when we call
->discard_folio so the folio remains dirty and uptodate. Hence
after a failed writeback due to allocation failure, we have the
situation where a dirty page has no backing store, and it seems a
future writeback can trip over that state.
Hence I suspect this commit first exposed the issue being reported
here:
commit e9c3a8e820ed0eeb2be05072f29f80d1b79f053b
Author: Darrick J. Wong <djwong@kernel.org>
Date: Mon May 16 15:27:38 2022 -0700
iomap: don't invalidate folios after writeback errors
XFS has the unique behavior (as compared to the other Linux filesystems)
that on writeback errors it will completely invalidate the affected
folio and force the page cache to reread the contents from disk. All
other filesystems leave the page mapped and up to date.
This is a rude awakening for user programs, since (in the case where
write fails but reread doesn't) file contents will appear to revert to
old disk contents with no notification other than an EIO on fsync. This
might have been annoying back in the days when iomap dealt with one page
at a time, but with multipage folios, we can now throw away *megabytes*
worth of data for a single write error.
On *most* Linux filesystems, a program can respond to an EIO on write by
redirtying the entire file and scheduling it for writeback. This isn't
foolproof, since the page that failed writeback is no longer dirty and
could be evicted, but programs that want to recover properly *also*
have to detect XFS and regenerate every write they've made to the file.
When running xfs/314 on arm64, I noticed a UAF when xfs_discard_folio
invalidates multipage folios that could be undergoing writeback. If,
say, we have a 256K folio caching a mix of written and unwritten
extents, it's possible that we could start writeback of the first (say)
64K of the folio and then hit a writeback error on the next 64K. We
then free the iop attached to the folio, which is really bad because
writeback completion on the first 64k will trip over the "blocks per
folio > 1 && !iop" assertion.
This can't be fixed by only invalidating the folio if writeback fails at
the start of the folio, since the folio is marked !uptodate, which trips
other assertions elsewhere. Get rid of the whole behavior entirely.
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 8fb9b2797fc5..94b53cbdefad 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -1387,7 +1387,6 @@ iomap_writepage_map(struct iomap_writepage_ctx *wpc,
if (wpc->ops->discard_folio)
wpc->ops->discard_folio(folio, pos);
if (!count) {
- folio_clear_uptodate(folio);
folio_unlock(folio);
goto done;
}
This commit removed the invalidation of the folio after discarding
it's backing store via ->discard_folio, hence allowing writeback of
dirty folios that have no storage reservation or delalloc extent
backing the dirty folio.
This, to me, looks like the underlying problem here. If we are not
invalidating the cached data on writeback allocation failure, we must
not punch out the storage reservation backing the dirty cached data
as we are going to retry the write at some point in the future and
hence still need the storage reservation for the dirty folio....
..... and removing ->discard_folio() then results in delalloc
extents that cannot be converted by writeback and so when we go to
reclaim the inode during cache eviction we now have delalloc extents
on the inode and that trips assert failures because writeback is
it assumes that writeback will always resolve delalloc extents
either by conversion or discarding them.
Ok, the simple solution trips over other assumptions. More
investigation required....
-Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply related [flat|nested] 4+ messages in thread* [Bug 217030] [Syzkaller & bisect] There is "xfs_bmapi_convert_delalloc" WARNING in v6.2-rc7 kernel
2023-02-13 10:57 [Bug 217030] New: [Syzkaller & bisect] There is "xfs_bmapi_convert_delalloc" WARNING in v6.2-rc7 kernel bugzilla-daemon
2023-02-13 23:01 ` Dave Chinner
@ 2023-02-13 23:01 ` bugzilla-daemon
2023-02-14 1:23 ` bugzilla-daemon
2 siblings, 0 replies; 4+ messages in thread
From: bugzilla-daemon @ 2023-02-13 23:01 UTC (permalink / raw)
To: linux-xfs
https://bugzilla.kernel.org/show_bug.cgi?id=217030
--- Comment #1 from Dave Chinner (david@fromorbit.com) ---
On Mon, Feb 13, 2023 at 10:57:31AM +0000, bugzilla-daemon@kernel.org wrote:
> Bisected and found the first bad commit is:
> 7348b322332d8602a4133f0b861334ea021b134a
> xfs: xfs_bmap_punch_delalloc_range() should take a byte range
I don't see how that commit makes any material difference to the
code; the ranges being punched are the same, just being passed in
different units.
The test filesystem image has a 1kB block size, and it has a bad CRC
in the root bnobt block, hence any attempt to allocate fails
immediately with a corrupt root btree block. The first write done
by the reproducer is an O_SYNC write, so it writes 0x1400 bytes into
the page cache (5 1kB blocks) and then internally the write() runs
fsync() which then fails to allocate the blocks in the writeback
path.
This writeback failure then calls ->discard_page() on the write range,
which punches out the delalloc block underneath the failed write.
At the same time, the other thread is doing a direct IO write, and
it sees there are dirty page cache pages over the page cache and it
attempts to writeback the page. It calls into ->map_blocks which
then sees a delalloc extent and tries to convert it. By the time
writeback has locked the folio, locked the ILOCK and runs the
delalloc extent conversion, the delalloc extent has been punched,
and it fails to find the delalloc extent to convert. It then issues
the WARN_ON() reported in this bug report.
I don't think this a failure that is a result of the commit that the
bisect identified - I think the race condition has been there for a
lot longer, and for some reason we've perturbed the code enough
to now expose the race condition occasionally. i.e. the
->discard_folio path is not invalidating the folio or marking it
with an error when we punch the backing store from under it, so it's
remaining dirty in the page cache but without any backing store for
writeback.
Hence I suspect the problem here is that we aren't calling
folio_set_error() on folios that we fail the first delalloc block
conversion on (i.e. count is zero) so this isn't operating like an
IO error. i.e. page_endio() calls folio_set_error() when a
writeback IO fails, and we are not doing that when we call
->discard_folio so the folio remains dirty and uptodate. Hence
after a failed writeback due to allocation failure, we have the
situation where a dirty page has no backing store, and it seems a
future writeback can trip over that state.
Hence I suspect this commit first exposed the issue being reported
here:
commit e9c3a8e820ed0eeb2be05072f29f80d1b79f053b
Author: Darrick J. Wong <djwong@kernel.org>
Date: Mon May 16 15:27:38 2022 -0700
iomap: don't invalidate folios after writeback errors
XFS has the unique behavior (as compared to the other Linux filesystems)
that on writeback errors it will completely invalidate the affected
folio and force the page cache to reread the contents from disk. All
other filesystems leave the page mapped and up to date.
This is a rude awakening for user programs, since (in the case where
write fails but reread doesn't) file contents will appear to revert to
old disk contents with no notification other than an EIO on fsync. This
might have been annoying back in the days when iomap dealt with one page
at a time, but with multipage folios, we can now throw away *megabytes*
worth of data for a single write error.
On *most* Linux filesystems, a program can respond to an EIO on write by
redirtying the entire file and scheduling it for writeback. This isn't
foolproof, since the page that failed writeback is no longer dirty and
could be evicted, but programs that want to recover properly *also*
have to detect XFS and regenerate every write they've made to the file.
When running xfs/314 on arm64, I noticed a UAF when xfs_discard_folio
invalidates multipage folios that could be undergoing writeback. If,
say, we have a 256K folio caching a mix of written and unwritten
extents, it's possible that we could start writeback of the first (say)
64K of the folio and then hit a writeback error on the next 64K. We
then free the iop attached to the folio, which is really bad because
writeback completion on the first 64k will trip over the "blocks per
folio > 1 && !iop" assertion.
This can't be fixed by only invalidating the folio if writeback fails at
the start of the folio, since the folio is marked !uptodate, which trips
other assertions elsewhere. Get rid of the whole behavior entirely.
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 8fb9b2797fc5..94b53cbdefad 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -1387,7 +1387,6 @@ iomap_writepage_map(struct iomap_writepage_ctx *wpc,
if (wpc->ops->discard_folio)
wpc->ops->discard_folio(folio, pos);
if (!count) {
- folio_clear_uptodate(folio);
folio_unlock(folio);
goto done;
}
This commit removed the invalidation of the folio after discarding
it's backing store via ->discard_folio, hence allowing writeback of
dirty folios that have no storage reservation or delalloc extent
backing the dirty folio.
This, to me, looks like the underlying problem here. If we are not
invalidating the cached data on writeback allocation failure, we must
not punch out the storage reservation backing the dirty cached data
as we are going to retry the write at some point in the future and
hence still need the storage reservation for the dirty folio....
..... and removing ->discard_folio() then results in delalloc
extents that cannot be converted by writeback and so when we go to
reclaim the inode during cache eviction we now have delalloc extents
on the inode and that trips assert failures because writeback is
it assumes that writeback will always resolve delalloc extents
either by conversion or discarding them.
Ok, the simple solution trips over other assumptions. More
investigation required....
-Dave.
--
You may reply to this email to add a comment.
You are receiving this mail because:
You are watching the assignee of the bug.
^ permalink raw reply related [flat|nested] 4+ messages in thread* [Bug 217030] [Syzkaller & bisect] There is "xfs_bmapi_convert_delalloc" WARNING in v6.2-rc7 kernel
2023-02-13 10:57 [Bug 217030] New: [Syzkaller & bisect] There is "xfs_bmapi_convert_delalloc" WARNING in v6.2-rc7 kernel bugzilla-daemon
2023-02-13 23:01 ` Dave Chinner
2023-02-13 23:01 ` [Bug 217030] " bugzilla-daemon
@ 2023-02-14 1:23 ` bugzilla-daemon
2 siblings, 0 replies; 4+ messages in thread
From: bugzilla-daemon @ 2023-02-14 1:23 UTC (permalink / raw)
To: linux-xfs
https://bugzilla.kernel.org/show_bug.cgi?id=217030
--- Comment #2 from xupengfe (pengfei.xu@intel.com) ---
Hi Dave,
Thanks a lot for your comments!
I just used bisect scripts to find the above commit. And I reverted the commit
and the issue was gone.
Maybe it's not accurate for the root cause.
Your comments could give more clue for the problem solving.
Thanks!
--
You may reply to this email to add a comment.
You are receiving this mail because:
You are watching the assignee of the bug.
^ permalink raw reply [flat|nested] 4+ messages in thread