Re: [PATCH] jbd2: assign different lock_class_key for different filesystem

Jan Kara
2025-10-20T09:31:41Z

On Sun 19-10-25 19:52:43, Tetsuo Handa wrote:
> syzbot is reporting possibility of deadlock due to sharing lock_class_key
> for jbd2_handle across ext4 and ocfs2. But one disk partition can't have
> two filesystems at the same time, and how locks in journal_t interacts
> with other filesystem specific locks can vary depending on filesystems.
> Therefore, lockdep should treat locks in journal_t different locks if
> the filesystem which allocated the journal_t differs.
>
> Reported-by: syzbot+6e493c165d26d6fcbf72@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=6e493c165d26d6fcbf72
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Tested-by: syzbot+6e493c165d26d6fcbf72@syzkaller.appspotmail.com

Thanks for debugging this. I agree with the idea of your solution but the
implementation is just ugly. Just let the filesystem pass the lockdep key
into jbd2_journal_init_dev() / jbd2_journal_init_inode() and make ext4 and
ocfs2 functions initializing the journal each have its own lock_class_key
declared and pass it to jbd2 functions. That way changes are much simpler
and also jbd2 doesn't have to be aware about which filesystems are using
it.

Honza

> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 33e7c08c9529..4e206209a476 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -5964,7 +5964,7 @@ static journal_t *ext4_open_dev_journal(struct super_block *sb,
> return ERR_CAST(bdev_file);
>
> journal = jbd2_journal_init_dev(file_bdev(bdev_file), sb->s_bdev, j_start,
> - j_len, sb->s_blocksize);
> + j_len, sb->s_blocksize, EXT4_SUPER_MAGIC);
> if (IS_ERR(journal)) {
> ext4_msg(sb, KERN_ERR, "failed to create device journal");
> errno = PTR_ERR(journal);
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index d480b94117cd..2f4fbd74cf76 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -1517,11 +1517,13 @@ static int journal_load_superblock(journal_t *journal)
> * superblock and initialize the journal_t object.
> */
>
> -static journal_t *journal_init_common(struct block_device *bdev,
> - struct block_device *fs_dev,
> - unsigned long long start, int len, int blocksize)
> +static journal_t *journal_init_common(struct block_device *bdev, struct block_device *fs_dev,
> + unsigned long long start, int len, int blocksize,
> + unsigned long fsmagic)
> {
> - static struct lock_class_key jbd2_trans_commit_key;
> + static struct lock_class_key jbd2_trans_commit_key_ext4;
> + static struct lock_class_key jbd2_trans_commit_key_ocfs2;
> + static struct lock_class_key jbd2_trans_commit_key_unknown;
> journal_t *journal;
> int err;
> int n;
> @@ -1547,20 +1549,49 @@ static journal_t *journal_init_common(struct block_device *bdev,
> init_waitqueue_head(&journal->j_wait_updates);
> init_waitqueue_head(&journal->j_wait_reserved);
> init_waitqueue_head(&journal->j_fc_wait);
> - mutex_init(&journal->j_abort_mutex);
> - mutex_init(&journal->j_barrier);
> - mutex_init(&journal->j_checkpoint_mutex);
> - spin_lock_init(&journal->j_revoke_lock);
> - spin_lock_init(&journal->j_list_lock);
> - spin_lock_init(&journal->j_history_lock);
> - rwlock_init(&journal->j_state_lock);
> + if (IS_ENABLED(CONFIG_LOCKDEP) && IS_ENABLED(CONFIG_EXT4_FS) &&
> + fsmagic == EXT4_SUPER_MAGIC) {
> + mutex_init(&journal->j_abort_mutex);
> + mutex_init(&journal->j_barrier);
> + mutex_init(&journal->j_checkpoint_mutex);
> + spin_lock_init(&journal->j_revoke_lock);
> + spin_lock_init(&journal->j_list_lock);
> + spin_lock_init(&journal->j_history_lock);
> + rwlock_init(&journal->j_state_lock);
> + } else if (IS_ENABLED(CONFIG_LOCKDEP) && IS_ENABLED(CONFIG_OCFS2_FS) &&
> + fsmagic == OCFS2_SUPER_MAGIC) {
> + mutex_init(&journal->j_abort_mutex);
> + mutex_init(&journal->j_barrier);
> + mutex_init(&journal->j_checkpoint_mutex);
> + spin_lock_init(&journal->j_revoke_lock);
> + spin_lock_init(&journal->j_list_lock);
> + spin_lock_init(&journal->j_history_lock);
> + rwlock_init(&journal->j_state_lock);
> + } else {
> + mutex_init(&journal->j_abort_mutex);
> + mutex_init(&journal->j_barrier);
> + mutex_init(&journal->j_checkpoint_mutex);
> + spin_lock_init(&journal->j_revoke_lock);
> + spin_lock_init(&journal->j_list_lock);
> + spin_lock_init(&journal->j_history_lock);
> + rwlock_init(&journal->j_state_lock);
> + }
>
> journal->j_commit_interval = (HZ * JBD2_DEFAULT_MAX_COMMIT_AGE);
> journal->j_min_batch_time = 0;
> journal->j_max_batch_time = 15000; /* 15ms */
> atomic_set(&journal->j_reserved_credits, 0);
> - lockdep_init_map(&journal->j_trans_commit_map, "jbd2_handle",
> - &jbd2_trans_commit_key, 0);
> + if (IS_ENABLED(CONFIG_LOCKDEP) && IS_ENABLED(CONFIG_EXT4_FS) &&
> + fsmagic == EXT4_SUPER_MAGIC)
> + lockdep_init_map(&journal->j_trans_commit_map, "jbd2_handle_ext4",
> + &jbd2_trans_commit_key_ext4, 0);
> + else if (IS_ENABLED(CONFIG_LOCKDEP) && IS_ENABLED(CONFIG_OCFS2_FS) &&
> + fsmagic == OCFS2_SUPER_MAGIC)
> + lockdep_init_map(&journal->j_trans_commit_map, "jbd2_handle_ocfs2",
> + &jbd2_trans_commit_key_ocfs2, 0);
> + else
> + lockdep_init_map(&journal->j_trans_commit_map, "jbd2_handle_unknown",
> + &jbd2_trans_commit_key_unknown, 0);
>
> /* The journal is marked for error until we succeed with recovery! */
> journal->j_flags = JBD2_ABORT;
> @@ -1631,6 +1662,7 @@ static journal_t *journal_init_common(struct block_device *bdev,
> * @start: Block nr Start of journal.
> * @len: Length of the journal in blocks.
> * @blocksize: blocksize of journalling device
> + * @fsmagic: filesystem magic number for lockdep annotation
> *
> * Returns: a newly created journal_t *
> *
> @@ -1638,13 +1670,13 @@ static journal_t *journal_init_common(struct block_device *bdev,
> * range of blocks on an arbitrary block device.
> *
> */
> -journal_t *jbd2_journal_init_dev(struct block_device *bdev,
> - struct block_device *fs_dev,
> - unsigned long long start, int len, int blocksize)
> +journal_t *jbd2_journal_init_dev(struct block_device *bdev, struct block_device *fs_dev,
> + unsigned long long start, int len, int blocksize,
> + unsigned long fsmagic)
> {
> journal_t *journal;
>
> - journal = journal_init_common(bdev, fs_dev, start, len, blocksize);
> + journal = journal_init_common(bdev, fs_dev, start, len, blocksize, fsmagic);
> if (IS_ERR(journal))
> return ERR_CAST(journal);
>
> @@ -1682,8 +1714,8 @@ journal_t *jbd2_journal_init_inode(struct inode *inode)
> inode->i_sb->s_blocksize_bits, inode->i_sb->s_blocksize);
>
> journal = journal_init_common(inode->i_sb->s_bdev, inode->i_sb->s_bdev,
> - blocknr, inode->i_size >> inode->i_sb->s_blocksize_bits,
> - inode->i_sb->s_blocksize);
> + blocknr, inode->i_size >> inode->i_sb->s_blocksize_bits,
> + inode->i_sb->s_blocksize, inode->i_sb->s_magic);
> if (IS_ERR(journal))
> return ERR_CAST(journal);
>
> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> index 43b9297fe8a7..56aa7ae4ec0b 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -1523,9 +1523,9 @@ extern void jbd2_journal_unlock_updates (journal_t *);
>
> void jbd2_journal_wait_updates(journal_t *);
>
> -extern journal_t * jbd2_journal_init_dev(struct block_device *bdev,
> - struct block_device *fs_dev,
> - unsigned long long start, int len, int bsize);
> +extern journal_t *jbd2_journal_init_dev(struct block_device *bdev, struct block_device *fs_dev,
> + unsigned long long start, int len, int bsize,
> + unsigned long fsmagic);
> extern journal_t * jbd2_journal_init_inode (struct inode *);
> extern int jbd2_journal_update_format (journal_t *);
> extern int jbd2_journal_check_used_features
> --
> 2.47.3
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR