linux-kernel - Re: [RFC v3 1/2] ext4: fast_commit: assert i_data

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <19b98d3901d.6853c0d25256967.6003033557062251155@linux.beauty>
Date: Wed, 07 Jan 2026 22:19:20 +0800
From: Li Chen <me@...ux.beauty>
To: "Zhang Yi" <yi.zhang@...weicloud.com>
Cc: "linux-ext4" <linux-ext4@...r.kernel.org>,
	"Theodore Ts'o" <tytso@....edu>,
	"Andreas Dilger" <adilger.kernel@...ger.ca>,
	"linux-kernel" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC v3 1/2] ext4: fast_commit: assert i_data_sem only before
 sleep

Hi Zhang,

Thanks a lot for your comments!

 ---- On Mon, 05 Jan 2026 20:18:42 +0800  Zhang Yi <yi.zhang@...weicloud.com> wrote --- 
 > Hi Li,
 > 
 > On 12/24/2025 11:29 AM, Li Chen wrote:
 > > ext4_fc_track_inode() can return without sleeping when
 > > EXT4_STATE_FC_COMMITTING is already clear. The lockdep assertion for
 > > ei->i_data_sem was done unconditionally before the wait loop, which can
 > > WARN in call paths that hold i_data_sem even though we never block. Move
 > > lockdep_assert_not_held(&ei->i_data_sem) into the actual sleep path,
 > > right before schedule().
 > > 
 > > Signed-off-by: Li Chen <me@...ux.beauty>
 > 
 > Thank you for the fix patch! However, the solution does not seem to fix
 > the issue. IIUC, the root cause of this issue is the following race
 > condition (show only one case), and it may cause a real ABBA dead lock
 > issue.
 > 
 > ext4_map_blocks()
 >  hold i_data_sem // <- A
 >  ext4_mb_new_blocks()
 >   ext4_dirty_inode()
 >                                  ext4_fc_commit()
 >                                   ext4_fc_perform_commit()
 >                                    set EXT4_STATE_FC_COMMITTING  <-B
 >                                    ext4_fc_write_inode_data()
 >                                    ext4_map_blocks()
 >                                     hold i_data_sem  // <- A
 >    ext4_fc_track_inode()
 >     wait EXT4_STATE_FC_COMMITTING  <- B
 >                                   jbd2_fc_end_commit()
 >                                    ext4_fc_cleanup()
 >                                     clear EXT4_STATE_FC_COMMITTING()
 > 
 > Postponing the lockdep assertion to the point where sleeping is actually
 > necessary does not resolve this deadlock issue, it merely masks the
 > problem, right?
 
I agree. Moving lockdep_assert_not_held(&ei->i_data_sem) closer to the
schedule() site can reduce spurious warnings (since the wait-bit pattern
rechecks the bit after prepare_to_wait()), but it does not remove the
underlying deadlock risk if we ever end up sleeping there, althoughI still 
haven't been able to reproduce this ABBA issue.

 > I currently don't quite understand why only ext4_fc_track_inode() needs
 > to wait for the inode being fast committed to be completed, instead of
 > adding it to the FC_Q_STAGING list like other tracking operations. So
 > now I don't have a good idea to fix this problem either.  Perhaps we
 > need to rethink the necessity of this waiting, or find a way to avoid
 > acquiring i_data_sem during fast commit.
 > 
 > Thanks,
 > Yi.
 > 
 > > ---
 > >  fs/ext4/fast_commit.c | 17 +++++++++--------
 > >  1 file changed, 9 insertions(+), 8 deletions(-)
 > > 
 > > diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
 > > index d0926967d086..b0c458082997 100644
 > > --- a/fs/ext4/fast_commit.c
 > > +++ b/fs/ext4/fast_commit.c
 > > @@ -566,13 +566,6 @@ void ext4_fc_track_inode(handle_t *handle, struct inode *inode)
 > >      if (ext4_test_mount_flag(inode->i_sb, EXT4_MF_FC_INELIGIBLE))
 > >          return;
 > >  
 > > -    /*
 > > -     * If we come here, we may sleep while waiting for the inode to
 > > -     * commit. We shouldn't be holding i_data_sem when we go to sleep since
 > > -     * the commit path needs to grab the lock while committing the inode.
 > > -     */
 > > -    lockdep_assert_not_held(&ei->i_data_sem);
 > > -
 > >      while (ext4_test_inode_state(inode, EXT4_STATE_FC_COMMITTING)) {
 > >  #if (BITS_PER_LONG < 64)
 > >          DEFINE_WAIT_BIT(wait, &ei->i_state_flags,
 > > @@ -586,8 +579,16 @@ void ext4_fc_track_inode(handle_t *handle, struct inode *inode)
 > >                     EXT4_STATE_FC_COMMITTING);
 > >  #endif
 > >          prepare_to_wait(wq, &wait.wq_entry, TASK_UNINTERRUPTIBLE);
 > > -        if (ext4_test_inode_state(inode, EXT4_STATE_FC_COMMITTING))
 > > +        if (ext4_test_inode_state(inode, EXT4_STATE_FC_COMMITTING)) {
 > > +            /*
 > > +             * We might sleep while waiting for the inode to commit.
 > > +             * We shouldn't be holding i_data_sem when we go to sleep
 > > +             * since the commit path may grab it while committing this
 > > +             * inode.
 > > +             */
 > > +            lockdep_assert_not_held(&ei->i_data_sem);
 > >              schedule();
 > > +        }
 > >          finish_wait(wq, &wait.wq_entry);
 > >      }
 > >  
 > 
 > 

Regards,
Li