ext4_try_to_expand_extra_isize() is called from __ext4_mark_inode_dirty() while holding an active jbd2 handle. During mount (!SB_ACTIVE), the expand path may move xattrs to external blocks and release ea_inodes via iput(). When !SB_ACTIVE, iput() calls write_inode_now() which acquires s_writepages_rwsem, creating a circular lock dependency: s_writepages_rwsem --> jbd2_handle --> xattr_sem --> s_writepages_rwsem This can be triggered via: ext4_process_orphan() -> ext4_truncate() -> ext4_mark_inode_dirty() -> ext4_try_to_expand_extra_isize() or: ext4_evict_inode() -> ext4_mark_inode_dirty() -> ext4_try_to_expand_extra_isize() Skip expansion when !SB_ACTIVE. This is a minor loss of functionality (extra isize won't grow for these inodes during mount), which e2fsck can resolve later if needed. Reported-by: syzbot+5d19358d7eb30ffb0cc5@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=5d19358d7eb30ffb0cc5 Fixes: c8585c6fcaf2 ("ext4: fix races between changing inode journal mode and ext4_writepages") Signed-off-by: Yun Zhou Reviewed-by: Jan Kara --- fs/ext4/inode.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index c2c2d6ac7f3d..09dcfb6bf48c 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -6458,6 +6458,16 @@ static int ext4_try_to_expand_extra_isize(struct inode *inode, if (ext4_test_inode_state(inode, EXT4_STATE_NO_EXPAND)) return -EOVERFLOW; + /* + * Skip expansion during mount (!SB_ACTIVE). Expanding extra isize + * may move xattrs to external blocks and release ea_inodes via iput. + * When !SB_ACTIVE, iput triggers write_inode_now() which acquires + * s_writepages_rwsem, causing a deadlock with the caller's active + * jbd2 handle (lock order: s_writepages_rwsem -> jbd2_handle). + */ + if (unlikely(!(inode->i_sb->s_flags & SB_ACTIVE))) + return -EBUSY; + /* * In nojournal mode, we can immediately attempt to expand * the inode. When journaled, we first need to obtain extra -- 2.43.0 An inode being evicted will never need its extra isize expanded. Set EXT4_STATE_NO_EXPAND before ext4_mark_inode_dirty() in ext4_evict_inode() to make this explicit and prevent any unnecessary work in ext4_try_to_expand_extra_isize(). This also provides defense-in-depth for the s_writepages_rwsem deadlock during mount-time orphan cleanup, ensuring the expand path is blocked for inodes under eviction regardless of how they are reached. Signed-off-by: Yun Zhou Reviewed-by: Jan Kara --- fs/ext4/inode.c | 1 + 1 file changed, 1 insertion(+) diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index 09dcfb6bf48c..1de0aaa28e63 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -264,6 +264,7 @@ void ext4_evict_inode(struct inode *inode) if (ext4_inode_is_fast_symlink(inode)) memset(EXT4_I(inode)->i_data, 0, sizeof(EXT4_I(inode)->i_data)); inode->i_size = 0; + ext4_set_inode_state(inode, EXT4_STATE_NO_EXPAND); err = ext4_mark_inode_dirty(handle, inode); if (err) { ext4_warning(inode->i_sb, -- 2.43.0 Calling iput() on EA inodes while holding xattr_sem or a jbd2 handle can trigger write_inode_now() -> ext4_writepages() -> s_writepages_rwsem, creating a lock ordering issue during mount (!SB_ACTIVE). Add ext4_put_ea_inode() which safely releases EA inode references: when SB_ACTIVE, it calls iput() directly (write_inode_now cannot be triggered); during mount (!SB_ACTIVE), it queues the inode on a per-sb lock-free llist and schedules a delayed worker (1 jiffie) to call iput() in a clean context without holding any ext4 locks. The delay allows multiple inodes to accumulate before the worker runs, reducing context switches. Convert the iput in ext4_xattr_block_set()'s "Drop the previous xattr block" path to use ext4_xattr_inode_array_free_deferred(), which releases EA inodes via ext4_put_ea_inode(). This path previously called ext4_xattr_inode_array_free() (synchronous iput) while holding xattr_sem and a jbd2 handle. The worker is flushed in ext4_put_super() before quota shutdown to ensure all pending EA inode cleanup completes while quota accounting is still active. Signed-off-by: Yun Zhou --- fs/ext4/ext4.h | 5 ++++ fs/ext4/super.c | 6 ++++ fs/ext4/xattr.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++- fs/ext4/xattr.h | 2 ++ 4 files changed, 86 insertions(+), 1 deletion(-) diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h index 94283a991e5c..e31d60f82a63 100644 --- a/fs/ext4/ext4.h +++ b/fs/ext4/ext4.h @@ -1706,6 +1706,11 @@ struct ext4_sb_info { struct ext4_es_stats s_es_stats; struct mb_cache *s_ea_block_cache; struct mb_cache *s_ea_inode_cache; + + /* Deferred iput for EA inodes to avoid lock ordering issues */ + struct llist_head s_ea_inode_to_free; + struct delayed_work s_ea_inode_work; + spinlock_t s_es_lock ____cacheline_aligned_in_smp; /* Journal triggers for checksum computation */ diff --git a/fs/ext4/super.c b/fs/ext4/super.c index 6a77db4d3124..5dd7c29a70bc 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -1303,6 +1303,8 @@ static void ext4_put_super(struct super_block *sb) &sb->s_uuid); ext4_unregister_li_request(sb); + /* Flush deferred EA inode iputs while quota is still active */ + flush_delayed_work(&sbi->s_ea_inode_work); ext4_quotas_off(sb, EXT4_MAXQUOTAS); destroy_workqueue(sbi->rsv_conversion_wq); @@ -5535,6 +5537,9 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb) needs_recovery = 0; } + init_llist_head(&sbi->s_ea_inode_to_free); + INIT_DELAYED_WORK(&sbi->s_ea_inode_work, ext4_ea_inode_work); + if (!test_opt(sb, NO_MBCACHE)) { sbi->s_ea_block_cache = ext4_xattr_create_cache(); if (!sbi->s_ea_block_cache) { @@ -5763,6 +5768,7 @@ failed_mount8: __maybe_unused if (EXT4_SB(sb)->rsv_conversion_wq) destroy_workqueue(EXT4_SB(sb)->rsv_conversion_wq); failed_mount_wq: + flush_delayed_work(&sbi->s_ea_inode_work); ext4_xattr_destroy_cache(sbi->s_ea_inode_cache); sbi->s_ea_inode_cache = NULL; diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c index 982a1f831e22..79de182e22e6 100644 --- a/fs/ext4/xattr.c +++ b/fs/ext4/xattr.c @@ -117,6 +117,8 @@ const struct xattr_handler * const ext4_xattr_handlers[] = { static int ext4_expand_inode_array(struct ext4_xattr_inode_array **ea_inode_array, struct inode *inode); +static void ext4_xattr_inode_array_free_deferred(struct super_block *sb, + struct ext4_xattr_inode_array *array); #ifdef CONFIG_LOCKDEP void ext4_xattr_inode_set_class(struct inode *ea_inode) @@ -2187,7 +2189,8 @@ ext4_xattr_block_set(handle_t *handle, struct inode *inode, ext4_xattr_release_block(handle, inode, bs->bh, &ea_inode_array, 0 /* extra_credits */); - ext4_xattr_inode_array_free(ea_inode_array); + ext4_xattr_inode_array_free_deferred(inode->i_sb, + ea_inode_array); } error = 0; @@ -3025,6 +3028,75 @@ void ext4_xattr_inode_array_free(struct ext4_xattr_inode_array *ea_inode_array) kfree(ea_inode_array); } +static void ext4_xattr_inode_array_free_deferred(struct super_block *sb, + struct ext4_xattr_inode_array *array) +{ + int idx; + + if (array == NULL) + return; + + for (idx = 0; idx < array->count; ++idx) + ext4_put_ea_inode(sb, array->inodes[idx]); + kfree(array); +} + +struct ext4_ea_iput_entry { + struct llist_node node; + struct inode *inode; +}; + +/* + * Worker function for deferred EA inode iput. Processes all inodes queued + * on s_ea_inode_to_free in a context free of xattr_sem/jbd2 handle locks. + */ +void ext4_ea_inode_work(struct work_struct *work) +{ + struct ext4_sb_info *sbi = container_of(to_delayed_work(work), + struct ext4_sb_info, + s_ea_inode_work); + struct llist_node *node = llist_del_all(&sbi->s_ea_inode_to_free); + struct llist_node *next; + + while (node) { + struct ext4_ea_iput_entry *entry = container_of(node, + struct ext4_ea_iput_entry, node); + next = node->next; + iput(entry->inode); + kfree(entry); + node = next; + } +} + +/* + * Release a VFS reference on an EA inode after ext4_xattr_inode_dec_ref() + * may have set i_nlink=0. Must be used instead of iput() in any context + * where xattr_sem or a jbd2 handle is held, because eviction of a nlink=0 + * inode can acquire those same locks. + * + * When SB_ACTIVE, eviction does not call write_inode_now() so direct + * iput() is safe. During mount (!SB_ACTIVE), defer to a workqueue. + * + * For EA inode references dropped without a preceding dec_ref (e.g., + * lookup-only paths where nlink remains >= 1), plain iput() is safe + * and preferred. + */ +void ext4_put_ea_inode(struct super_block *sb, struct inode *inode) +{ + struct ext4_ea_iput_entry *entry; + + if (!inode) + return; + if (sb->s_flags & SB_ACTIVE) { + iput(inode); + return; + } + entry = kmalloc(sizeof(*entry), GFP_NOFS | __GFP_NOFAIL); + entry->inode = inode; + llist_add(&entry->node, &EXT4_SB(sb)->s_ea_inode_to_free); + schedule_delayed_work(&EXT4_SB(sb)->s_ea_inode_work, 1); +} + /* * ext4_xattr_block_cache_insert() * diff --git a/fs/ext4/xattr.h b/fs/ext4/xattr.h index 1fedf44d4fb6..52074537dce5 100644 --- a/fs/ext4/xattr.h +++ b/fs/ext4/xattr.h @@ -190,6 +190,8 @@ extern int ext4_xattr_delete_inode(handle_t *handle, struct inode *inode, struct ext4_xattr_inode_array **array, int extra_credits); extern void ext4_xattr_inode_array_free(struct ext4_xattr_inode_array *array); +extern void ext4_ea_inode_work(struct work_struct *work); +extern void ext4_put_ea_inode(struct super_block *sb, struct inode *inode); extern int ext4_expand_extra_isize_ea(struct inode *inode, int new_extra_isize, struct ext4_inode *raw_inode, handle_t *handle); -- 2.43.0 Convert all remaining iput() calls on EA inodes that execute under xattr_sem or a jbd2 handle to use ext4_put_ea_inode(). With i_nlink>=1 and !SB_ACTIVE, a direct iput() would trigger write_inode_now() -> s_writepages_rwsem, creating a lock ordering violation with the caller's active jbd2 handle. Converted sites and why defer is necessary: - ext4_xattr_inode_inc_ref_all() cleanup: dec_ref undoes the failed inc_ref, but the EA inode may be shared so i_nlink remains 1. - ext4_xattr_inode_dec_ref_all() ENOMEM fallback: ext4_expand_inode_array() failed before dec_ref is called, i_nlink=1, jbd2 handle active. - ext4_xattr_inode_lookup_create() out_err: may be a cache-found inode where inc_ref failed; i_nlink remains 1. - ext4_xattr_set_entry() old_ea_inode: dec_ref was called but the EA inode may be shared by other xattr blocks, so i_nlink remains 1. - ext4_xattr_block_set() new block path: dec_ref drops the "extra" ref but inc_ref_all added another, so i_nlink stays 1. - ext4_xattr_block_set() cleanup: on success no dec_ref was called (i_nlink=1); on error dec_ref may leave i_nlink=1 if shared. - ext4_xattr_ibody_set() error path: dec_ref on a cache-found EA inode may leave i_nlink=1 if shared. - ext4_xattr_ibody_set() success path: newly stored EA inode with i_nlink=1, just releasing the lookup reference. - ext4_xattr_delete_inode() quota loop: iget for quota accounting only, no dec_ref called, i_nlink=1, jbd2 handle is active. Direct iput() calls in pure lookup paths (ext4_xattr_inode_get, ext4_xattr_inode_cache_find, tmp_inode in ext4_xattr_block_set) are left unchanged -- these do not hold a jbd2 handle or xattr_sem. Signed-off-by: Yun Zhou --- fs/ext4/xattr.c | 40 ++++++++++++++++++++++++++++------------ 1 file changed, 28 insertions(+), 12 deletions(-) diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c index 79de182e22e6..08c1bdd5133d 100644 --- a/fs/ext4/xattr.c +++ b/fs/ext4/xattr.c @@ -1079,6 +1079,13 @@ static int ext4_xattr_inode_inc_ref(handle_t *handle, struct inode *ea_inode) return ext4_xattr_inode_update_ref(handle, ea_inode, 1); } +/* + * Decrement on-disk reference count of an EA inode. If refcount reaches 0, + * i_nlink is cleared and the inode is added to the orphan list. Callers + * must use ext4_put_ea_inode() (not iput) to release the VFS reference + * afterwards, since iput on a nlink=0 inode triggers eviction which may + * deadlock if called under xattr_sem or an active jbd2 handle. + */ static int ext4_xattr_inode_dec_ref(handle_t *handle, struct inode *ea_inode) { return ext4_xattr_inode_update_ref(handle, ea_inode, -1); @@ -1106,10 +1113,10 @@ static int ext4_xattr_inode_inc_ref_all(handle_t *handle, struct inode *parent, err = ext4_xattr_inode_inc_ref(handle, ea_inode); if (err) { ext4_warning_inode(ea_inode, "inc ref error %d", err); - iput(ea_inode); + ext4_put_ea_inode(parent->i_sb, ea_inode); goto cleanup; } - iput(ea_inode); + ext4_put_ea_inode(parent->i_sb, ea_inode); } return 0; @@ -1135,7 +1142,8 @@ static int ext4_xattr_inode_inc_ref_all(handle_t *handle, struct inode *parent, if (err) ext4_warning_inode(ea_inode, "cleanup dec ref error %d", err); - iput(ea_inode); + /* i_nlink may remain 1 if shared; defer for !SB_ACTIVE safety */ + ext4_put_ea_inode(parent->i_sb, ea_inode); } return saved_err; } @@ -1203,7 +1211,8 @@ ext4_xattr_inode_dec_ref_all(handle_t *handle, struct inode *parent, if (err) { ext4_warning_inode(ea_inode, "Expand inode array err=%d", err); - iput(ea_inode); + /* i_nlink=1 (dec_ref not yet called); handle active */ + ext4_put_ea_inode(parent->i_sb, ea_inode); continue; } @@ -1507,7 +1516,7 @@ static struct inode *ext4_xattr_inode_create(handle_t *handle, if (ext4_xattr_inode_dec_ref(handle, ea_inode)) ext4_warning_inode(ea_inode, "cleanup dec ref error %d", err); - iput(ea_inode); + ext4_put_ea_inode(inode->i_sb, ea_inode); return ERR_PTR(err); } @@ -1617,7 +1626,8 @@ static struct inode *ext4_xattr_inode_lookup_create(handle_t *handle, ea_inode->i_ino, true /* reusable */); return ea_inode; out_err: - iput(ea_inode); + /* May be cache-found inode with i_nlink=1 (inc_ref failed) */ + ext4_put_ea_inode(inode->i_sb, ea_inode); ext4_xattr_inode_free_quota(inode, NULL, value_len); return ERR_PTR(err); } @@ -1850,7 +1860,8 @@ static int ext4_xattr_set_entry(struct ext4_xattr_info *i, ret = 0; out: - iput(old_ea_inode); + /* old_ea_inode had dec_ref; may still have i_nlink=1 if shared */ + ext4_put_ea_inode(inode->i_sb, old_ea_inode); return ret; } @@ -2152,7 +2163,8 @@ ext4_xattr_block_set(handle_t *handle, struct inode *inode, ext4_warning_inode(ea_inode, "dec ref error=%d", error); - iput(ea_inode); + /* i_nlink stays 1 (inc_ref_all added a ref) */ + ext4_put_ea_inode(inode->i_sb, ea_inode); ea_inode = NULL; } @@ -2206,7 +2218,8 @@ ext4_xattr_block_set(handle_t *handle, struct inode *inode, ext4_xattr_inode_free_quota(inode, ea_inode, i_size_read(ea_inode)); } - iput(ea_inode); + /* success: i_nlink=1; error+dec_ref: may still be 1 if shared */ + ext4_put_ea_inode(inode->i_sb, ea_inode); } if (ce) mb_cache_entry_put(ea_block_cache, ce); @@ -2288,7 +2301,8 @@ int ext4_xattr_ibody_set(handle_t *handle, struct inode *inode, ext4_xattr_inode_free_quota(inode, ea_inode, i_size_read(ea_inode)); - iput(ea_inode); + /* cache-found ea_inode may retain i_nlink=1 */ + ext4_put_ea_inode(inode->i_sb, ea_inode); } return error; } @@ -2300,7 +2314,8 @@ int ext4_xattr_ibody_set(handle_t *handle, struct inode *inode, header->h_magic = cpu_to_le32(0); ext4_clear_inode_state(inode, EXT4_STATE_XATTR); } - iput(ea_inode); + /* ea_inode has i_nlink=1 (new ref just stored in xattr entry) */ + ext4_put_ea_inode(inode->i_sb, ea_inode); return 0; } @@ -2989,7 +3004,8 @@ int ext4_xattr_delete_inode(handle_t *handle, struct inode *inode, continue; ext4_xattr_inode_free_quota(inode, ea_inode, le32_to_cpu(entry->e_value_size)); - iput(ea_inode); + /* no dec_ref yet but i_nlink=1; handle is active */ + ext4_put_ea_inode(inode->i_sb, ea_inode); } } -- 2.43.0