[PATCH v4 0/1] iomap: fix invalid folio access after folio_end_read()

[PATCH v4 0/1] iomap: fix invalid folio access after folio_end_read()

[Joanne Koong]

This patch is on top of Christian's vfs.fixes tree.

Changelog
---------
v3:
https://lore.kernel.org/linux-fsdevel/20260116200427.1016177-1-joannelkoong@gmail.com/
* Make commit message more descriptive (Christoph)
* Also remove "+1" bias (Matthew)
* Account for non-readahead reads as well (me)

v2:
https://lore.kernel.org/linux-fsdevel/20260116015452.757719-1-joannelkoong@gmail.com/
* Fix incorrect spacing (Matthew)
* Reword commit title and message (Matthew)

v1:
https://lore.kernel.org/linux-fsdevel/20260114180255.3043081-1-joannelkoong@gmail.com/
* Invalidate ctx->cur_folio instead of retaining readahead caller refcount (Matthew)

Joanne Koong (1):
  iomap: fix invalid folio access after folio_end_read()

 fs/iomap/buffered-io.c | 69 ++++++++++++++++++++++--------------------
 1 file changed, 37 insertions(+), 32 deletions(-)

-- 
2.47.3

[PATCH v4 1/1] iomap: fix invalid folio access after folio_end_read()

[Joanne Koong]

If the folio does not have an iomap_folio_state (ifs) attached and the
folio gets read in by the filesystem's IO helper, folio_end_read() will
be called by the IO helper at any time. For this case, we cannot access
the folio after dispatching it to the IO helper, eg subsequent accesses
like

	if (ctx->cur_folio &&
                    offset_in_folio(ctx->cur_folio, iter->pos) == 0) {

are incorrect.

Fix these invalid accesses by invalidating ctx->cur_folio if all bytes
of the folio have been read in by the IO helper.

This allows us to also remove the +1 bias added for the ifs case. The
bias was previously added to ensure that if all bytes are read in, the
IO helper does not end the read on the folio until iomap has decremented
the bias.

Fixes: b2f35ac4146d ("iomap: add caller-provided callbacks for read and readahead")
Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
---
 fs/iomap/buffered-io.c | 69 ++++++++++++++++++++++--------------------
 1 file changed, 37 insertions(+), 32 deletions(-)

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 6beb876658c0..f8937fd5ee8e 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -409,8 +409,6 @@ static void iomap_read_init(struct folio *folio)
 	struct iomap_folio_state *ifs = folio->private;
 
 	if (ifs) {
-		size_t len = folio_size(folio);
-
 		/*
 		 * ifs->read_bytes_pending is used to track how many bytes are
 		 * read in asynchronously by the IO helper. We need to track
@@ -418,23 +416,19 @@ static void iomap_read_init(struct folio *folio)
 		 * reading in all the necessary ranges of the folio and can end
 		 * the read.
 		 *
-		 * Increase ->read_bytes_pending by the folio size to start, and
-		 * add a +1 bias. We'll subtract the bias and any uptodate /
-		 * zeroed ranges that did not require IO in iomap_read_end()
-		 * after we're done processing the folio.
+		 * Increase ->read_bytes_pending by the folio size to start.
+		 * We'll subtract any uptodate / zeroed ranges that did not
+		 * require IO in iomap_read_end() after we're done processing
+		 * the folio.
 		 *
 		 * We do this because otherwise, we would have to increment
 		 * ifs->read_bytes_pending every time a range in the folio needs
 		 * to be read in, which can get expensive since the spinlock
 		 * needs to be held whenever modifying ifs->read_bytes_pending.
-		 *
-		 * We add the bias to ensure the read has not been ended on the
-		 * folio when iomap_read_end() is called, even if the IO helper
-		 * has already finished reading in the entire folio.
 		 */
 		spin_lock_irq(&ifs->state_lock);
 		WARN_ON_ONCE(ifs->read_bytes_pending != 0);
-		ifs->read_bytes_pending = len + 1;
+		ifs->read_bytes_pending = folio_size(folio);
 		spin_unlock_irq(&ifs->state_lock);
 	}
 }
@@ -465,11 +459,9 @@ static void iomap_read_end(struct folio *folio, size_t bytes_submitted)
 
 		/*
 		 * Subtract any bytes that were initially accounted to
-		 * read_bytes_pending but skipped for IO. The +1 accounts for
-		 * the bias we added in iomap_read_init().
+		 * read_bytes_pending but skipped for IO.
 		 */
-		ifs->read_bytes_pending -=
-			(folio_size(folio) + 1 - bytes_submitted);
+		ifs->read_bytes_pending -= folio_size(folio) - bytes_submitted;
 
 		/*
 		 * If !ifs->read_bytes_pending, this means all pending reads by
@@ -483,18 +475,30 @@ static void iomap_read_end(struct folio *folio, size_t bytes_submitted)
 		spin_unlock_irq(&ifs->state_lock);
 		if (end_read)
 			folio_end_read(folio, uptodate);
-	} else if (!bytes_submitted) {
+	} else {
 		/*
-		 * If there were no bytes submitted, this means we are
-		 * responsible for unlocking the folio here, since no IO helper
-		 * has taken ownership of it. If there were bytes submitted,
-		 * then the IO helper will end the read via
-		 * iomap_finish_folio_read().
+		 * If a folio without an ifs is submitted to the IO helper, the
+		 * read must be on the entire folio and the IO helper takes
+		 * ownership of the folio. This means we should only enter
+		 * iomap_read_end() for the !ifs case if no bytes were submitted
+		 * to the IO helper, in which case we are responsible for
+		 * unlocking the folio here.
 		 */
+		WARN_ON_ONCE(bytes_submitted);
 		folio_unlock(folio);
 	}
 }
 
+static void iomap_read_submit_and_end(struct iomap_read_folio_ctx *ctx,
+		size_t bytes_submitted)
+{
+	if (ctx->ops->submit_read)
+		ctx->ops->submit_read(ctx);
+
+	if (ctx->cur_folio)
+		iomap_read_end(ctx->cur_folio, bytes_submitted);
+}
+
 static int iomap_read_folio_iter(struct iomap_iter *iter,
 		struct iomap_read_folio_ctx *ctx, size_t *bytes_submitted)
 {
@@ -502,6 +506,7 @@ static int iomap_read_folio_iter(struct iomap_iter *iter,
 	loff_t pos = iter->pos;
 	loff_t length = iomap_length(iter);
 	struct folio *folio = ctx->cur_folio;
+	size_t folio_len = folio_size(folio);
 	size_t poff, plen;
 	loff_t pos_diff;
 	int ret;
@@ -515,8 +520,7 @@ static int iomap_read_folio_iter(struct iomap_iter *iter,
 
 	ifs_alloc(iter->inode, folio, iter->flags);
 
-	length = min_t(loff_t, length,
-			folio_size(folio) - offset_in_folio(folio, pos));
+	length = min_t(loff_t, length, folio_len - offset_in_folio(folio, pos));
 	while (length) {
 		iomap_adjust_read_range(iter->inode, folio, &pos, length, &poff,
 				&plen);
@@ -542,7 +546,15 @@ static int iomap_read_folio_iter(struct iomap_iter *iter,
 			ret = ctx->ops->read_folio_range(iter, ctx, plen);
 			if (ret)
 				return ret;
+
 			*bytes_submitted += plen;
+			/*
+			 * If the entire folio has been read in by the IO
+			 * helper, then the helper owns the folio and will end
+			 * the read on it.
+			 */
+			if (*bytes_submitted == folio_len)
+				ctx->cur_folio = NULL;
 		}
 
 		ret = iomap_iter_advance(iter, plen);
@@ -572,10 +584,7 @@ void iomap_read_folio(const struct iomap_ops *ops,
 		iter.status = iomap_read_folio_iter(&iter, ctx,
 				&bytes_submitted);
 
-	if (ctx->ops->submit_read)
-		ctx->ops->submit_read(ctx);
-
-	iomap_read_end(folio, bytes_submitted);
+	iomap_read_submit_and_end(ctx, bytes_submitted);
 }
 EXPORT_SYMBOL_GPL(iomap_read_folio);
 
@@ -636,11 +645,7 @@ void iomap_readahead(const struct iomap_ops *ops,
 		iter.status = iomap_readahead_iter(&iter, ctx,
 					&cur_bytes_submitted);
 
-	if (ctx->ops->submit_read)
-		ctx->ops->submit_read(ctx);
-
-	if (ctx->cur_folio)
-		iomap_read_end(ctx->cur_folio, cur_bytes_submitted);
+	iomap_read_submit_and_end(ctx, cur_bytes_submitted);
 }
 EXPORT_SYMBOL_GPL(iomap_readahead);
 
-- 
2.47.3

Re: [PATCH v4 1/1] iomap: fix invalid folio access after folio_end_read()

[Christoph Hellwig]

> -	if (ctx->ops->submit_read)
> -		ctx->ops->submit_read(ctx);
> -
> -	iomap_read_end(folio, bytes_submitted);
> +	iomap_read_submit_and_end(ctx, bytes_submitted);

Can you drop this cleanup for now?  I think it's actually useful,
but it should be in a separate patch, and creates a conflict with
my iomap PI series.

The actual fix looks great and simplified the code nicely!

Re: [PATCH v4 1/1] iomap: fix invalid folio access after folio_end_read()

[Matthew Wilcox]

On Sun, Jan 25, 2026 at 09:46:30PM -0800, Christoph Hellwig wrote:
> > -	if (ctx->ops->submit_read)
> > -		ctx->ops->submit_read(ctx);
> > -
> > -	iomap_read_end(folio, bytes_submitted);
> > +	iomap_read_submit_and_end(ctx, bytes_submitted);
> 
> Can you drop this cleanup for now?  I think it's actually useful,
> but it should be in a separate patch, and creates a conflict with
> my iomap PI series.
> 
> The actual fix looks great and simplified the code nicely!

I don't think it's just a cleanup -- I think it's a bug fix.  But, yes,
it should be a separate patch because it's a separate bug.  That bug
can be hit if the folio passed to iomap_read_folio() covers more than
one extent, the first call to iomap_iter() succeeds, and then the second
one fails.  Now we have a folio with a positive read_pending that will
never become zero, so we'll never unlock the folio.

Not sure how we'd write an fstest that would exercise this ...

Re: [PATCH v4 1/1] iomap: fix invalid folio access after folio_end_read()

[Christoph Hellwig]

On Mon, Jan 26, 2026 at 02:55:31PM +0000, Matthew Wilcox wrote:
> > Can you drop this cleanup for now?  I think it's actually useful,
> > but it should be in a separate patch, and creates a conflict with
> > my iomap PI series.
> > 
> > The actual fix looks great and simplified the code nicely!
> 
> I don't think it's just a cleanup -- I think it's a bug fix.  But, yes,
> it should be a separate patch because it's a separate bug.  That bug
> can be hit if the folio passed to iomap_read_folio() covers more than
> one extent, the first call to iomap_iter() succeeds, and then the second
> one fails.  Now we have a folio with a positive read_pending that will
> never become zero, so we'll never unlock the folio.

Oh, I missed it added a condition for the read_folio case.  Another
reason to split the fix from the (otherwise nice) refactoring.  And
fixing it directly in read_folio also helps with the conflict avoidance.

Re: [PATCH v4 1/1] iomap: fix invalid folio access after folio_end_read()

[Joanne Koong]

On Mon, Jan 26, 2026 at 6:55 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Sun, Jan 25, 2026 at 09:46:30PM -0800, Christoph Hellwig wrote:
> > > -   if (ctx->ops->submit_read)
> > > -           ctx->ops->submit_read(ctx);
> > > -
> > > -   iomap_read_end(folio, bytes_submitted);
> > > +   iomap_read_submit_and_end(ctx, bytes_submitted);
> >
> > Can you drop this cleanup for now?  I think it's actually useful,
> > but it should be in a separate patch, and creates a conflict with
> > my iomap PI series.
> >
> > The actual fix looks great and simplified the code nicely!
>
> I don't think it's just a cleanup -- I think it's a bug fix.  But, yes,
> it should be a separate patch because it's a separate bug.  That bug
> can be hit if the folio passed to iomap_read_folio() covers more than
> one extent, the first call to iomap_iter() succeeds, and then the second
> one fails.  Now we have a folio with a positive read_pending that will
> never become zero, so we'll never unlock the folio.

I don't think there's a separate bug. The number of bytes submitted
is tracked per-folio across iterations/mappings. If the first call to
iomap_iter() succeeds and the second one fails, iomap_read_folio()
still calls iomap_read_end() and decrements ifs->read_bytes_pending /
does any folio unlocking it might need to do.

This change to iomap_read_folio() is a fix for the original bug (eg
for folios without an ifs, the IO helper might have already called
folio_end_read(), so ctx->cur_folio() needs to be used instead of the
direct folio pointer).

I'll drop the change for the new iomap_read_submit_and_end() helper
and submit that later as a separate patch to Christian's tree.

Thanks,
Joanne


>
> Not sure how we'd write an fstest that would exercise this ...

Re: [PATCH v4 1/1] iomap: fix invalid folio access after folio_end_read()

[Matthew Wilcox]

On Mon, Jan 26, 2026 at 01:36:21PM -0800, Joanne Koong wrote:
> I don't think there's a separate bug. The number of bytes submitted
> is tracked per-folio across iterations/mappings. If the first call to
> iomap_iter() succeeds and the second one fails, iomap_read_folio()
> still calls iomap_read_end() and decrements ifs->read_bytes_pending /
> does any folio unlocking it might need to do.
> 
> This change to iomap_read_folio() is a fix for the original bug (eg
> for folios without an ifs, the IO helper might have already called
> folio_end_read(), so ctx->cur_folio() needs to be used instead of the
> direct folio pointer).

Oh yes, I got confused.  You're right.

> I'll drop the change for the new iomap_read_submit_and_end() helper
> and submit that later as a separate patch to Christian's tree.

I don't think it deserves to be factored out into a separate function,
honestly.

Re: [PATCH v4 1/1] iomap: fix invalid folio access after folio_end_read()

[Joanne Koong]

On Mon, Jan 26, 2026 at 1:42 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> > I'll drop the change for the new iomap_read_submit_and_end() helper
> > and submit that later as a separate patch to Christian's tree.
>
> I don't think it deserves to be factored out into a separate function,
> honestly.

I'll drop this altogether then.

Thanks,
Joanne