Re: [PATCH v2] fs/buffer: serialize set_buffer_uptodate against concurrent clears

[Chao S]

On Thu 30-04-26 09:28:13, Jan Kara wrote:
> But it would be actually very welcome if you took the work, went through
> all the places using end_buffer_write_sync() and converted them in
> filesystem-by-filesystem to check for IO error by checking
> buffer_write_io_error() instead of buffer_uptodate() and then we can drop
> clear_buffer_uptodate() call from end_buffer_write_sync().

Sorry for the long delay, I've been working on some patches and a paper
deadline.  Also, you were right about v2, and I have dropped it.  I
would like to
take the conversion work.

Before writing it, I found your "fs: Fix missed inode write during fsync"
series, v4 of 16 July.  It removes four of the sites I had on my list, so
I will base this on top of it once it lands.  Say if you would rather
sequence it differently.

(end_buffer_write_sync() is now bh_end_write(), per b20f15420f78.  Current
names below; line numbers against v7.2-rc1.)

The sites, one patch each:

  core     fs/buffer.c:621 (mmb_sync), :2803 (__sync_dirty_buffer)
  adfs     fs/adfs/dir.c:194
  ext2     fs/ext2/xattr.c:772
  ext4     fs/ext4/ext4_jbd2.c:399, fs/ext4/mmp.c:52
  omfs     fs/omfs/inode.c:148, :162
  exfat    fs/exfat/misc.c:190
  fat      fs/fat/misc.c:358
  ocfs2    fs/ocfs2/buffer_head_io.c:69, :449, fs/ocfs2/journal.c:675, :691
  gfs2     fs/gfs2/log.c:110, :324
  jbd2     fs/jbd2/commit.c:880
  removal  fs/buffer.c:210 and :444

Conversions first, removal last.  The list is short because
__sync_dirty_buffer() and mmb_sync() test internally and return -EIO, so
the ~40 callers that use the return value need no edit of their own.

I would like your suggestions on the following.

1. BH_Write_EIO is sticky.  It is cleared only on rewrite
(fs/buffer.c:1196), and only if BH_Req was already set, which
BUFFER_FLAGS_DISCARD clears on its own.  So a converted test can see a
stale error.  I would fix this first, either by making the clear at :1196
unconditional for REQ_OP_WRITE, or by adding BH_Write_EIO to
BUFFER_FLAGS_DISCARD.  Which do you prefer?

2. jbd2: log_bufs is a mixed list.  Commit descriptors come from
journal_end_buffer_io_sync(), which never sets BH_Write_EIO; revoke
descriptors from write_dirty_buffer(), so bh_end_write().  I would convert
fs/jbd2/commit.c:880 to the disjunct !buffer_uptodate(bh) ||
buffer_write_io_error(bh).  The alternative is to add
mark_buffer_write_io_error() to journal_end_buffer_io_sync() and convert
all three jbd2 sites.  Which do you prefer?

3. bh_end_async_write() has the same clear at fs/buffer.c:444.  I would
remove both, which needs the gfs2 patch first since gfs2 reads that bit at
fs/gfs2/log.c:110 and :324.  I can also leave the async half out.

4. ocfs2: fs/ocfs2/journal.c:691 sits inside an outer
if (!buffer_uptodate(bh)) at :675, so dropping the clear stops ocfs2 going
read-only after a failed metadata checkpoint.  I would convert both sites
and Cc ocfs2-devel.

Build-tested only so far.

Chao

On Thu, Apr 30, 2026 at 3:28 AM Jan Kara <jack@xxxxxxx> wrote:
>
> On Thu 30-04-26 01:36:45, Chao Shi wrote:
> > A WARN_ON_ONCE(!buffer_uptodate(bh)) in mark_buffer_dirty() is
> > reachable from the buffered write path on a block device when the
> > underlying device returns I/O errors at high density.  Reproduced
> > by fuzzing an NVMe controller (FEMU) that returns crafted error
> > completions for a sustained workload from /dev/nvme0n1.
> >
> > The contract documented at set_buffer_uptodate() in
> > include/linux/buffer_head.h reads:
> >
> >   Any other serialization (with IO errors or whatever that might
> >   clear the bit) has to come from other state (eg BH_Lock).
> >
> > In fs/buffer.c, BH_Uptodate can be cleared from four I/O completion
> > callbacks: __end_buffer_read_notouch, end_buffer_write_sync,
> > end_buffer_async_read, end_buffer_async_write.
> >
> > end_buffer_async_read() runs with BH_Lock held throughout, so its
> > clear is already serialized against any caller that also holds
> > BH_Lock around set_buffer_uptodate(); the call may in fact be
> > redundant, but addressing that is independent of this fix.
> >
> > end_buffer_write_sync() likewise holds BH_Lock while it clears
> > BH_Uptodate on the write-error path.  Removing that clear would
> > change long-standing buffer-cache I/O-error semantics and is out
> > of scope here.
> >
> > The race is therefore between block_commit_write() and
> > end_buffer_write_sync():
> >
> >   CPU A: block_commit_write              CPU B: end_buffer_write_sync
> >          (folio lock held, BH_Lock NOT)         (BH_Lock held)
> >     set_buffer_uptodate(bh);
> >                                            clear_buffer_uptodate(bh);
> >                                            unlock_buffer(bh);
> >     mark_buffer_dirty(bh);  /* WARN */
> >
> > CPU B observes the contract; CPU A does not.  With one side unlocked
> > the serialization is one-sided and ineffective: CPU A's set can be
> > immediately followed by CPU B's clear, tripping the WARN_ON_ONCE.
> > In the fuzzing reproducer, write-error completions are frequent
> > (visible as repeated "lost async page write" and per-LBA write
> > failures); buffer I/O completion callbacks on the write-error path
> > (e.g. end_buffer_write_sync, end_buffer_async_write) clear
> > BH_Uptodate while holding BH_Lock.
> >
> > The bug is not benign: a not-uptodate buffer can be marked dirty
> > and subsequently written back; depending on whether the buffer was
> > fully or partially covered by the user write, this can leave on-disk
> > content that does not match the intended buffered write state.
> >
> > Fix this by taking BH_Lock around set_buffer_uptodate() +
> > mark_buffer_dirty() in block_commit_write(), so both sides of the
> > contract use the documented serialization.
> >
> > Found by FuzzNvme (Syzkaller with FEMU fuzzing framework).
> >
> > Acked-by: Sungwoo Kim <iam@xxxxxxxxxxxx>
> > Acked-by: Dave Tian <daveti@xxxxxxxxxx>
> > Acked-by: Weidong Zhu <weizhu@xxxxxxx>
> > Signed-off-by: Chao Shi <coshi036@xxxxxxxxx>
>
> Thanks for trying but this basically just silences the warning without
> addressing the real problem. Sure enough it silences the warning in
> mark_buffer_dirty() but effectively it just papers over the real problem -
> the IO completion with error can come the moment you unlock the bh and it
> will happily clear the uptodate bit, resulting in the same "dirty but not
> uptodate" invalid buffer state. So I actually prefer keeping things as they
> currently are so that we are reminded there is this unresolved issue.
>
> But it would be actually very welcome if you took the work, went through
> all the places using end_buffer_write_sync() and converted them in
> filesystem-by-filesystem to check for IO error by checking
> buffer_write_io_error() instead of buffer_uptodate() and then we can drop
> clear_buffer_uptodate() call from end_buffer_write_sync(). Yes, it is much
> more work but also much more useful.
>
>                                                                 Honza
>
> > ---
> > Hi Matthew, Hi Jan,
> >
> > Thanks for the review on v1.  v2 takes the feedback, quick notes below.
> >
> > To Matthew:
> >
> > You were right that the v1's timing diagram named the wrong racer.
> >
> > The actual race is with end_buffer_write_sync() on the write-error
> > path, as Jan pointed out -- it also clears BH_Uptodate under BH_Lock,
> > but block_commit_write()'s else branch was reaching set_buffer_uptodate
> > without BH_Lock, leaving the serialization one-sided.  v2's commit
> > message and in-code comment now name end_buffer_write_sync() as the
> > racer.
> >
> > To Jan:
> >
> > Thanks for confirming the racer and for the historical context on the
> > dirty + !uptodate state question.  v2 keeps the fix scoped to taking
> > BH_Lock in block_commit_write(); the broader semantic question and any
> > change to end_buffer_write_sync()'s clear is left out of scope here.
> >
> > The redundant lock_buffer/unlock_buffer that v1 added to the
> > buffer_new branch of __block_write_begin_int() is also dropped in v2 --
> > that bh has no in-flight async I/O so no race exists there.
> >
> > v1 thread for context:
> > https://lore.kernel.org/all/20260426020137.1221985-1-coshi036@xxxxxxxxx/
> >
> > Thanks,
> > Chao
> >
> > Changes in v2:
> >   - Drop the lock_buffer/unlock_buffer added in v1 to the buffer_new
> >     branch of __block_write_begin_int(): that bh is freshly BH_New
> >     and has no in-flight async I/O on it, so no race exists at that
> >     site.
> >   - Rewrite the commit message and the in-code comment to identify
> >     end_buffer_write_sync() as the actual racer, not
> >     end_buffer_async_read() as v1 claimed; end_buffer_async_read()
> >     holds BH_Lock across its clear so a caller that also holds
> >     BH_Lock would already be serialized.
> >   - Reference the BH_Lock contract at set_buffer_uptodate() in
> >     include/linux/buffer_head.h explicitly.
> >   - Drop verbose line-number citations and the WARN stack dump from
> >     the commit message; tighten wording around reproducer evidence
> >     and on-disk impact.
> >
> > v1: https://lore.kernel.org/all/20260426020137.1221985-1-coshi036@xxxxxxxxx/
> >
> >  fs/buffer.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> >
> > diff --git a/fs/buffer.c b/fs/buffer.c
> > index 4d7f84e77d2..6fddb2f1e7c 100644
> > --- a/fs/buffer.c
> > +++ b/fs/buffer.c
> > @@ -2104,8 +2104,22 @@ void block_commit_write(struct folio *folio, size_t from, size_t to)
> >                       if (!buffer_uptodate(bh))
> >                               partial = true;
> >               } else {
> > +                     /*
> > +                      * Per the contract documented at set_buffer_uptodate()
> > +                      * in include/linux/buffer_head.h, callers must hold
> > +                      * BH_Lock to serialize against concurrent clears of
> > +                      * BH_Uptodate.  Holding only the folio lock is not
> > +                      * sufficient: a concurrent end_buffer_write_sync() on
> > +                      * the write-error path clears BH_Uptodate while
> > +                      * holding BH_Lock; without BH_Lock here the clear can
> > +                      * land between set_buffer_uptodate() and
> > +                      * mark_buffer_dirty(), tripping the WARN_ON_ONCE in
> > +                      * mark_buffer_dirty().
> > +                      */
> > +                     lock_buffer(bh);
> >                       set_buffer_uptodate(bh);
> >                       mark_buffer_dirty(bh);
> > +                     unlock_buffer(bh);
> >               }
> >               if (buffer_new(bh))
> >                       clear_buffer_new(bh);
> >
> > base-commit: ffe69af1f87fa77da975ad4b0b093d48c3cbe6c3
> > --
> > 2.43.0
> >
> --
> Jan Kara <jack@xxxxxxxx>
> SUSE Labs, CR