| 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
| ||
|
Message-ID: <40b04c68-377b-4770-bff1-ecff8afa70e9@huawei.com>
Date: Mon, 13 Jan 2025 22:16:30 +0800
From: Baokun Li <libaokun1@...wei.com>
To: Harshad Shirwadkar <harshadshirwadkar@...il.com>
CC: <linux-ext4@...r.kernel.org>, <tytso@....edu>, <jack@...e.cz>,
<harshads@...gle.com>, Baokun Li <libaokun1@...wei.com>, Yang Erkun
<yangerkun@...wei.com>
Subject: Re: [PATCH v7 9/9] ext4: hold s_fc_lock while during fast commit
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@...il.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
Powered by blists - more mailing lists