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: <e3465e09-0b6f-419c-9af5-00e750448e53@huaweicloud.com>
Date: Mon, 5 Jan 2026 20:18:42 +0800
From: Zhang Yi <yi.zhang@...weicloud.com>
To: Li Chen <me@...ux.beauty>, linux-ext4@...r.kernel.org
Cc: Theodore Ts'o <tytso@....edu>, Andreas Dilger <adilger.kernel@...ger.ca>,
 linux-kernel@...r.kernel.org
Subject: Re: [RFC v3 1/2] ext4: fast_commit: assert i_data_sem only before
 sleep

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 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);
>  	}
>