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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <fwsxrb7ugi5zeosugo6hyjdbhw36ppa5kekfi6n7we2vvi3r7m@ljrizqoagsg7>
Date: Mon, 20 Oct 2025 11:31:31 +0200
From: Jan Kara <jack@...e.cz>
To: Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
Cc: Theodore Ts'o <tytso@....edu>, 
	Andreas Dilger <adilger.kernel@...ger.ca>, Jan Kara <jack@...e.com>, 
	"linux-ext4@...r.kernel.org" <linux-ext4@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] jbd2: assign different lock_class_key for different
 filesystem

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@...kaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=6e493c165d26d6fcbf72
> Signed-off-by: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
> Tested-by: syzbot+6e493c165d26d6fcbf72@...kaller.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@...e.com>
SUSE Labs, CR

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ