Re: [PATCH v7 9/9] ext4: hold s_fc_lock while during fast commit

[Baokun Li <libaokun1@huawei.com>]

Hi Harshad,

On 2024/8/18 12:03, Harshad Shirwadkar wrote:
> Leaving s_fc_lock in between during commit in ext4_fc_perform_commit()
> function leaves room for subtle concurrency bugs where ext4_fc_del() may
> delete an inode from the fast commit list, leaving list in an inconsistent
> state. Also, this patch converts s_fc_lock to mutex type so that it can be
> held when kmem_cache_* functions are called.
>
> Signed-off-by: Harshad Shirwadkar <harshadshirwadkar@gmail.com>
> ---
>   fs/ext4/ext4.h        |  2 +-
>   fs/ext4/fast_commit.c | 91 +++++++++++++++++--------------------------
>   fs/ext4/super.c       |  2 +-
>   3 files changed, 38 insertions(+), 57 deletions(-)
>
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 4ecb63f95..a1acd34ff 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -1748,7 +1748,7 @@ struct ext4_sb_info {
>   	 * following fields:
>   	 * ei->i_fc_list, s_fc_dentry_q, s_fc_q, s_fc_bytes, s_fc_bh.
>   	 */
> -	spinlock_t s_fc_lock;
> +	struct mutex s_fc_lock;
>   	struct buffer_head *s_fc_bh;
>   	struct ext4_fc_stats s_fc_stats;
>   	tid_t s_fc_ineligible_tid;
> diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
> index 7525450f1..c3627efd9 100644
> --- a/fs/ext4/fast_commit.c
> +++ b/fs/ext4/fast_commit.c
> @@ -236,9 +236,9 @@ void ext4_fc_del(struct inode *inode)
>   	if (ext4_fc_disabled(inode->i_sb))
>   		return;
>   
> -	spin_lock(&EXT4_SB(inode->i_sb)->s_fc_lock);
> +	mutex_lock(&EXT4_SB(inode->i_sb)->s_fc_lock);
>   	if (list_empty(&ei->i_fc_list) && list_empty(&ei->i_fc_dilist)) {
> -		spin_unlock(&EXT4_SB(inode->i_sb)->s_fc_lock);
> +		mutex_unlock(&EXT4_SB(inode->i_sb)->s_fc_lock);
>   		return;
>   	}
>   
> @@ -266,7 +266,7 @@ void ext4_fc_del(struct inode *inode)
>   	 * dentry create references, since it is not needed to log it anyways.
>   	 */
>   	if (list_empty(&ei->i_fc_dilist)) {
> -		spin_unlock(&sbi->s_fc_lock);
> +		mutex_unlock(&sbi->s_fc_lock);
>   		return;
>   	}
>   
> @@ -276,7 +276,7 @@ void ext4_fc_del(struct inode *inode)
>   	list_del_init(&fc_dentry->fcd_dilist);
>   
>   	WARN_ON(!list_empty(&ei->i_fc_dilist));
> -	spin_unlock(&sbi->s_fc_lock);
> +	mutex_unlock(&sbi->s_fc_lock);
>   
>   	if (fc_dentry->fcd_name.name &&
>   		fc_dentry->fcd_name.len > DNAME_INLINE_LEN)
> @@ -306,10 +306,10 @@ void ext4_fc_mark_ineligible(struct super_block *sb, int reason, handle_t *handl
>   				sbi->s_journal->j_running_transaction->t_tid : 0;
>   		read_unlock(&sbi->s_journal->j_state_lock);
>   	}
> -	spin_lock(&sbi->s_fc_lock);
> +	mutex_lock(&sbi->s_fc_lock);
>   	if (tid_gt(tid, sbi->s_fc_ineligible_tid))
>   		sbi->s_fc_ineligible_tid = tid;
> -	spin_unlock(&sbi->s_fc_lock);
> +	mutex_unlock(&sbi->s_fc_lock);
>   	WARN_ON(reason >= EXT4_FC_REASON_MAX);
>   	sbi->s_fc_stats.fc_ineligible_reason_count[reason]++;
>   }
> @@ -349,14 +349,14 @@ static int ext4_fc_track_template(
>   	if (!enqueue)
>   		return ret;
>   
> -	spin_lock(&sbi->s_fc_lock);
> +	mutex_lock(&sbi->s_fc_lock);
>   	if (list_empty(&EXT4_I(inode)->i_fc_list))
>   		list_add_tail(&EXT4_I(inode)->i_fc_list,
>   				(sbi->s_journal->j_flags & JBD2_FULL_COMMIT_ONGOING ||
>   				 sbi->s_journal->j_flags & JBD2_FAST_COMMIT_ONGOING) ?
>   				&sbi->s_fc_q[FC_Q_STAGING] :
>   				&sbi->s_fc_q[FC_Q_MAIN]);
> -	spin_unlock(&sbi->s_fc_lock);
> +	mutex_unlock(&sbi->s_fc_lock);
>   
>   	return ret;
>   }
> @@ -414,7 +414,8 @@ static int __track_dentry_update(struct inode *inode, void *arg, bool update)
>   	}
>   	node->fcd_name.len = dentry->d_name.len;
>   	INIT_LIST_HEAD(&node->fcd_dilist);
> -	spin_lock(&sbi->s_fc_lock);
> +	INIT_LIST_HEAD(&node->fcd_list);
> +	mutex_lock(&sbi->s_fc_lock);
>   	if (sbi->s_journal->j_flags & JBD2_FULL_COMMIT_ONGOING ||
>   		sbi->s_journal->j_flags & JBD2_FAST_COMMIT_ONGOING)
>   		list_add_tail(&node->fcd_list,
> @@ -435,7 +436,7 @@ static int __track_dentry_update(struct inode *inode, void *arg, bool update)
>   		WARN_ON(!list_empty(&ei->i_fc_dilist));
>   		list_add_tail(&node->fcd_dilist, &ei->i_fc_dilist);
>   	}
> -	spin_unlock(&sbi->s_fc_lock);
> +	mutex_unlock(&sbi->s_fc_lock);
>   	spin_lock(&ei->i_fc_lock);
>   
>   	return 0;
> @@ -955,15 +956,15 @@ static int ext4_fc_submit_inode_data_all(journal_t *journal)
>   	struct ext4_inode_info *ei;
>   	int ret = 0;
>   
> -	spin_lock(&sbi->s_fc_lock);
> +	mutex_lock(&sbi->s_fc_lock);
>   	list_for_each_entry(ei, &sbi->s_fc_q[FC_Q_MAIN], i_fc_list) {
> -		spin_unlock(&sbi->s_fc_lock);
> +		mutex_unlock(&sbi->s_fc_lock);
>   		ret = jbd2_submit_inode_data(journal, ei->jinode);
>   		if (ret)
>   			return ret;
> -		spin_lock(&sbi->s_fc_lock);
> +		mutex_lock(&sbi->s_fc_lock);
>   	}
> -	spin_unlock(&sbi->s_fc_lock);
> +	mutex_unlock(&sbi->s_fc_lock);
>   
We're also seeing a similar race condition here. This issue was encountered
while running `kvm-xfstests -c ext4/adv -C 500 generic/241`:

     P1                |         P2
----------------------------------------------------
                            evict
                             ext4_evict_inode
                              ext4_free_inode
                               ext4_clear_inode
                                ext4_fc_del(inode)
ext4_sync_file
  ext4_fsync_journal
   ext4_fc_commit
    ext4_fc_perform_commit
     ext4_fc_submit_inode_data_all
      -- spin_lock(&sbi->s_fc_lock);
       list_for_each_entry(i_fc_list)
         -- spin_unlock(&sbi->s_fc_lock);
                                -- spin_lock(&sbi->s_fc_lock)
                                  if (!list_empty(&ei->i_fc_list))
list_del_init(&ei->i_fc_list);
                                -- spin_unlock(&sbi->s_fc_lock);
jbd2_free_inode(EXT4_I(inode)->jinode)
                                EXT4_I(inode)->jinode = NULL
          jbd2_submit_inode_data
           journal->j_submit_inode_data_buffers
            ext4_journal_submit_inode_data_buffers
             ext4_should_journal_data(jinode->i_vfs_inode)
              // a. jinode may use-after-free !!!
              ext4_inode_journal_mode(inode)
               EXT4_JOURNAL(inode)
                (inode)->i_sb
                 // b. inode may null-ptr-deref !!!
         -- spin_lock(&sbi->s_fc_lock);
      -- spin_unlock(&sbi->s_fc_lock);

By the way, the WARN_ON added in patch 5 can detect this issue without
enabling KASAN, but patch 5 also introduced softlocks and other UAFs.


Regards,
Baokun