iomap: fix error handling regressions
Subsystems (4)
v3 cover iomap: fix error handling regressions
Fix three independent error handling regressions in iomap:
- clear the writeback context after a failed ->writeback_submit() call, preventing an already completed ioend from being submitted again;
- preserve an iomap iteration error when iomap_fiemap() has a pending extent to emit; and
- preserve a direct I/O data path error when sub-block tail zeroing succeeds.
The three fixes have no ordering dependencies and can be applied or backported independently. Each issue was reproduced before and after its respective fix.
Patch 2 is not a fix and carries no stable tag: it adds an XFS error tag (requested by Christoph) that lets patch 1's ->writeback_submit() failure be injected without the boot-param hack used for the original reproduction. The corresponding xfstest (list corruption caught via CONFIG_LIST_HARDENED on an unfixed kernel, clean writeback failure on a fixed one) was posted to [email protected]:
https://lore.kernel.org/all/[email protected]/
Changes since v2:
- Patch 1: no code change; picked up Christoph's Reviewed-by.
- Patch 2: fixed the alignment of the new XFS_ERRTAG_WB_COW_CONVERT_ERROR define; picked up Reviewed-by tags from Darrick and Christoph.
- Patch 3: no change.
- Patch 4: dropped the reformatting of the iomap_dio_zero() prototype, keeping the iomap two-tab indent style (Christoph); added the "# v6.12" stable version tag (Darrick); picked up Reviewed-by tags from Darrick and Pankaj.
Changes since v1:
- Patch 1: reworded the added comment to fit 80 columns (Christoph); described the reproducer in the commit message (fault-injected ->writeback_submit() failure on a reflinked XFS file, reliably hits list_add double add on the unfixed kernel); documented why the final ->writeback_submit() call in iomap_writepages() needs no equivalent fix, since wpc is never reused after it; picked up the version-scoped stable tag Darrick suggested; picked up Brian's Reviewed-by.
- Patch 2: new, added per Christoph's request.
- Patch 3 (was 2/3): no code change; picked up the version-scoped stable tag Darrick suggested; picked up Reviewed-by tags from Brian, Darrick, and Christoph.
- Patch 4 (was 3/3): reworked per Christoph's suggestion to make iomap_dio_zero() return void and drop the handling of its return value, since it can only fail via a can't-happen WARN_ON_ONCE() assert; this also resolves Darrick's question about the local variable's type. Brian's v1 Reviewed-by was dropped due to the rework.
v2: https://lore.kernel.org/all/[email protected]/
v1: https://lore.kernel.org/all/[email protected]/
Andrea Parri (4):
iomap: don't resubmit an ioend after ->writeback_submit() failed
xfs: add an error tag to inject a ->writeback_submit() failure
iomap: don't lose a fiemap iteration error when emitting the last
extent
iomap: don't lose a failed direct I/O bio's error when zeroing the
tail
fs/iomap/direct-io.c | 16 +++++----------- fs/iomap/fiemap.c | 8 ++++---- fs/iomap/ioend.c | 10 +++++++++- fs/xfs/libxfs/xfs_errortag.h | 6 ++++-- fs/xfs/xfs_reflink.c | 5 +++++ 5 files changed, 27 insertions(+), 18 deletions(-)
base-commit: 93f51579e7df248780214094418f205253383cc5
Signature · 1 line
2.53.0
PATCH v3 1/4 iomap: don't resubmit an ioend after ->writeback_submit() failed
iomap_add_to_ioend() submits the pending ioend through ->writeback_submit() before allocating a new one for the current range. When the submission fails the helper completes the ioend with an error, but iomap_add_to_ioend() returns the error without clearing wpc->wb_ctx. iomap_writepages() then submits whatever wpc->wb_ctx points to, so the already completed ioend is submitted a second time.
For XFS the second bio_endio() lands in xfs_end_bio(), which list_add_tail()s the already linked ioend into ip->i_ioend_list. This corrupts the list and leaves a use-after-free/double-free window against the ioend completion worker. Reproduced with a fault-injected ->writeback_submit() failure on a reflinked XFS file with several CoW writeback ranges in flight: the unfixed kernel hits repeated "list_add double add" warnings from __list_add_valid_or_report(), the fixed kernel fails writeback cleanly.
Clear wpc->wb_ctx when ->writeback_submit() fails. The old iomap_submit_ioend() cleared the context unconditionally; that clear was lost when submission moved to iomap_ioend_writeback_submit(). The final ->writeback_submit() call in iomap_writepages() needs no equivalent fix:
it is the last thing the function does before returning, and every caller allocates its iomap_writepage_ctx on the stack for a single call, so wpc->wb_ctx is never read again afterwards.
--- fs/iomap/ioend.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
fs/iomap/ioend.c +9 -1
@@ -246,8 +246,16 @@ ssize_t iomap_add_to_ioend(struct iomap_writepage_ctx *wpc, struct folio *folio, new_ioend:if (ioend) { error = wpc->ops->writeback_submit (wpc,0 ); -if (error) +if (error) { +/* +* ->writeback_submit() completed the ioend +* with an error, so drop the stale context; +* iomap_writepages() would otherwise submit +* it a second time. +*/ + wpc->wb_ctx =NULL ;return error; + } } wpc->wb_ctx = ioend =iomap_alloc_ioend (wpc, pos, ioend_flags); }
Signature · 1 line
2.53.0
PATCH v3 2/4 xfs: add an error tag to inject a ->writeback_submit() failure
In reply to Andrea ParriThe only in-tree way for xfs_writeback_submit() to fail is a failing xfs_reflink_convert_cow(), which requires a shared (reflinked) CoW extent. There was previously no way to exercise that failure path from userspace, which made it hard to write an xfstest for it, e.g. for "iomap: don't resubmit an ioend after ->writeback_submit() failed".
Add XFS_ERRTAG_WB_COW_CONVERT_ERROR to force xfs_reflink_convert_cow() to fail with -EIO, so that ->writeback_submit() failure and its callers' error handling can be exercised with error injection alone, without needing a corrupted or racy COW fork.
--- fs/xfs/libxfs/xfs_errortag.h | 6 ++++-- fs/xfs/xfs_reflink.c | 5 +++++ 2 files changed, 9 insertions(+), 2 deletions(-)
fs/xfs/libxfs/xfs_errortag.h +4 -2
@@ -75,7 +75,8 @@#define XFS_ERRTAG_METAFILE_RESV_CRITICAL 45#define XFS_ERRTAG_FORCE_ZERO_RANGE 46#define XFS_ERRTAG_ZONE_RESET 47 -#define XFS_ERRTAG_MAX 48 +#define XFS_ERRTAG_WB_COW_CONVERT_ERROR 48 +#define XFS_ERRTAG_MAX 49/* * Random factors for above tags, 1 means always, 2 means 1/2 time, etc.
@@ -137,7 +138,8 @@ XFS_ERRTAG(WRITE_DELAY_MS, write_delay_ms, 3000) \ XFS_ERRTAG(EXCHMAPS_FINISH_ONE, exchmaps_finish_one, 1) \ XFS_ERRTAG(METAFILE_RESV_CRITICAL, metafile_resv_crit, 4) \ XFS_ERRTAG(FORCE_ZERO_RANGE, force_zero_range, 4) \ -XFS_ERRTAG(ZONE_RESET, zone_reset, 1) +XFS_ERRTAG(ZONE_RESET, zone_reset, 1) \ +XFS_ERRTAG(WB_COW_CONVERT_ERROR, wb_cow_convert_error, 1)#endif /* XFS_ERRTAG */ #endif /* __XFS_ERRORTAG_H_ */
fs/xfs/xfs_reflink.c +5 -0
@@ -34,6 +34,8 @@#include "xfs_rtalloc.h" #include "xfs_rtgroup.h" #include "xfs_metafile.h" +#include "xfs_errortag.h" +#include "xfs_error.h" /* * Copy on Write of Shared Blocks
@@ -346,6 +348,9 @@ xfs_reflink_convert_cow(ASSERT (count !=0 ); +if (XFS_TEST_ERROR (mp,XFS_ERRTAG_WB_COW_CONVERT_ERROR )) +return -EIO ; +xfs_ilock (ip,XFS_ILOCK_EXCL ); error =xfs_reflink_convert_cow_locked (ip, offset_fsb, count_fsb);xfs_iunlock (ip,XFS_ILOCK_EXCL );
Signature · 1 line
2.53.0
PATCH v3 3/4 iomap: don't lose a fiemap iteration error when emitting the last extent
In reply to Andrea Parriiomap_fiemap() emits extents one behind: iomap_fiemap_iter() flushes the previous extent and remembers the current one, and the remembered extent is written with FIEMAP_EXTENT_LAST after the iteration loop.
That final flush overwrites ret, so when ->iomap_begin() fails partway through the iteration the error is replaced by the result of iomap_to_fiemap() (zero on success) and iomap_fiemap() returns success with a truncated extent list whose last entry is wrongly marked as the last extent in the file. The pre-iomap_iter code returned the error from inside the loop, before flushing the pending extent.
Check for the iteration error before flushing the pending extent, so that real errors are propagated and only a successful iteration emits the final FIEMAP_EXTENT_LAST extent. -ENOENT (no mapping) is still not an error, and the pending extent is still emitted in that case.
--- fs/iomap/fiemap.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
fs/iomap/fiemap.c +4 -4
@@ -76,15 +76,15 @@ int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fi,while ((ret =iomap_iter (&iter, ops)) >0 ) iter.status =iomap_fiemap_iter (&iter, fi, &prev); +/* inode with no (attribute) mapping will give ENOENT */ +if (ret <0 && ret != -ENOENT ) +return ret; +if (prev.type !=IOMAP_HOLE ) { ret =iomap_to_fiemap (fi, &prev,FIEMAP_EXTENT_LAST );if (ret <0 )return ret; } - -/* inode with no (attribute) mapping will give ENOENT */ -if (ret <0 && ret != -ENOENT ) -return ret;return 0 ; }EXPORT_SYMBOL_GPL (iomap_fiemap);
Signature · 1 line
2.53.0
PATCH v3 4/4 iomap: don't lose a failed direct I/O bio's error when zeroing the tail
In reply to Andrea Parriiomap_dio_bio_iter() falls through to the sub-block tail zeroing when the data bio submission fails, so that the rest of the block is still zeroed and stale data is not exposed. The zeroing result was assigned to ret, which overwrote the submission error with the successful zeroing result (zero) and the failed write was reported as success.
iomap_dio_zero() can only return an error from a can't-happen WARN_ON_ONCE() (nr_vecs exceeding BIO_MAX_VECS, which the existing comment there says "shall never be reached" for any in-tree filesystem), so it isn't a real runtime failure worth reporting to userspace, let alone one worth losing the actual submission error for. Make iomap_dio_zero() return void and drop the error handling at both call sites instead of threading the result through a separate variable.
--- fs/iomap/direct-io.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-)
fs/iomap/direct-io.c +5 -11
@@ -296,7 +296,7 @@ u32 iomap_finish_ioend_direct(struct iomap_ioend *ioend)return vec_count; } -static int iomap_dio_zero (const struct iomap_iter *iter,struct iomap_dio *dio, +static void iomap_dio_zero (const struct iomap_iter *iter,struct iomap_dio *dio,loff_t pos,unsigned len) {struct inode *inode =file_inode (dio->iocb->ki_filp);
@@ -305,14 +305,14 @@ static int iomap_dio_zero(const struct iomap_iter *iter, struct iomap_dio *dio,int nr_vecs =max (1 ,i_blocksize (inode) /folio_size (zero_folio));if (!len) -return 0 ; +return ;/* * This limit shall never be reached as most filesystems have a * maximum blocksize of 64k. */ if (WARN_ON_ONCE (nr_vecs >BIO_MAX_VECS )) -return -EINVAL ; +return ; bio =iomap_dio_alloc_bio (iter, dio, nr_vecs,REQ_OP_WRITE |REQ_SYNC |REQ_IDLE );
@@ -328,8 +328,6 @@ static int iomap_dio_zero(const struct iomap_iter *iter, struct iomap_dio *dio, len -= io_len; }iomap_dio_submit_bio (iter, dio, bio, pos); - -return 0 ; } static ssize_t iomap_dio_bio_iter_one(struct iomap_iter *iter,
@@ -541,10 +539,7 @@ static int iomap_dio_bio_iter(struct iomap_iter *iter, struct iomap_dio *dio)if (need_zeroout) {/* zero out from the start of the block to the write offset */ pad = pos & (fs_block_size -1 ); - - ret =iomap_dio_zero (iter, dio, pos - pad, pad); -if (ret) - goto out; +iomap_dio_zero (iter, dio, pos - pad, pad); } do {
@@ -582,8 +577,7 @@ static int iomap_dio_bio_iter(struct iomap_iter *iter, struct iomap_dio *dio)/* zero out from the end of the write to the end of the block */ pad = pos & (fs_block_size -1 );if (pad) - ret =iomap_dio_zero (iter, dio, pos, - fs_block_size - pad); +iomap_dio_zero (iter, dio, pos, fs_block_size - pad); } out:/* Undo iter limitation to current extent */
Signature · 1 line
2.53.0
Re: [PATCH v3 4/4] iomap: don't lose a failed direct I/O bio's error when zeroing the tail
Looks good:
Quoted 10 lines · Andrea Parri
Fix three independent error handling regressions in iomap:
- clear the writeback context after a failed ->writeback_submit() call, preventing an already completed ioend from being submitted again;
- preserve an iomap iteration error wheniomap_fiemap()has a pending extent to emit; and
- preserve a direct I/O data path error when sub-block tail zeroing succeeds.[...]
Applied to the vfs-7.4.iomap branch of the vfs/vfs.git tree. Patches in the vfs-7.4.iomap branch should appear in linux-next soon.
Please report any outstanding bugs that were missed during review in a new review to the original patch series allowing us to drop it.
It's encouraged to provide Acked-bys and Reviewed-bys even though the patch has now been applied. If possible patch trailers will be updated.
Note that commit hashes shown below are subject to change due to rebase, trailer updates or similar. If in doubt, please check the listed branch.
tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git branch: vfs-7.4.iomap
[1/4] iomap: don't resubmit an ioend after ->writeback_submit() failed
https://git.kernel.org/vfs/vfs/c/9646682717e6 [2/4] xfs: add an error tag to inject a ->writeback_submit() failure https://git.kernel.org/vfs/vfs/c/e50b5a3028d0 [3/4] iomap: don't lose a fiemap iteration error when emitting the last extent https://git.kernel.org/vfs/vfs/c/ec2874845c99 [4/4] iomap: don't lose a failed direct I/O bio's error when zeroing the tail https://git.kernel.org/vfs/vfs/c/750148e10873