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@xxxxxxxxx>
---
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
