[PATCH 0/4] reiserfs: fixes

[PATCH 0/4] reiserfs: fixes

[Christian Brauner]

Hey Christoph & Jan,

A series of smaller fixes for reiserfs including one deadlock reported
by syzbot (albeit with a bogus bisection). Plan would be to get this
merged within -rc6.

I've actually tested all this with xfstests. With and without this
patch series the same 18 tests fail.

Thanks!
Christian


---
base-commit: 94f6f0550c625fab1f373bb86a6669b45e9748b3
change-id: 20231009-vfs-fixes-reiserfs-3402fe7abb94

[PATCH 1/4] reiserfs: user superblock as holder for journal device

[Christian Brauner]

I see no reason to use the journal as the holder of the block device.
The superblock should be used. In the case were the journal and main
device are the same we can easily reclaim because the same holder is
used.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/reiserfs/journal.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/fs/reiserfs/journal.c b/fs/reiserfs/journal.c
index 015bfe4e4524..b9d9bf26d108 100644
--- a/fs/reiserfs/journal.c
+++ b/fs/reiserfs/journal.c
@@ -2591,12 +2591,7 @@ static void release_journal_dev(struct super_block *super,
 			       struct reiserfs_journal *journal)
 {
 	if (journal->j_dev_bd != NULL) {
-		void *holder = NULL;
-
-		if (journal->j_dev_bd->bd_dev != super->s_dev)
-			holder = journal;
-
-		blkdev_put(journal->j_dev_bd, holder);
+		blkdev_put(journal->j_dev_bd, super);
 		journal->j_dev_bd = NULL;
 	}
 }
@@ -2606,7 +2601,6 @@ static int journal_init_dev(struct super_block *super,
 			    const char *jdev_name)
 {
 	blk_mode_t blkdev_mode = BLK_OPEN_READ;
-	void *holder = journal;
 	int result;
 	dev_t jdev;
 
@@ -2621,10 +2615,8 @@ static int journal_init_dev(struct super_block *super,
 
 	/* there is no "jdev" option and journal is on separate device */
 	if ((!jdev_name || !jdev_name[0])) {
-		if (jdev == super->s_dev)
-			holder = NULL;
-		journal->j_dev_bd = blkdev_get_by_dev(jdev, blkdev_mode, holder,
-						      NULL);
+		journal->j_dev_bd = blkdev_get_by_dev(jdev, blkdev_mode, super,
+						      &fs_holder_ops);
 		if (IS_ERR(journal->j_dev_bd)) {
 			result = PTR_ERR(journal->j_dev_bd);
 			journal->j_dev_bd = NULL;
@@ -2638,8 +2630,8 @@ static int journal_init_dev(struct super_block *super,
 		return 0;
 	}
 
-	journal->j_dev_bd = blkdev_get_by_path(jdev_name, blkdev_mode, holder,
-					       NULL);
+	journal->j_dev_bd = blkdev_get_by_path(jdev_name, blkdev_mode, super,
+					       &fs_holder_ops);
 	if (IS_ERR(journal->j_dev_bd)) {
 		result = PTR_ERR(journal->j_dev_bd);
 		journal->j_dev_bd = NULL;

-- 
2.34.1

Re: [PATCH 1/4] reiserfs: user superblock as holder for journal device

[Jan Kara]

On Mon 09-10-23 14:33:38, Christian Brauner wrote:
> I see no reason to use the journal as the holder of the block device.
> The superblock should be used. In the case were the journal and main
> device are the same we can easily reclaim because the same holder is
> used.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Looks sane. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/reiserfs/journal.c | 18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/reiserfs/journal.c b/fs/reiserfs/journal.c
> index 015bfe4e4524..b9d9bf26d108 100644
> --- a/fs/reiserfs/journal.c
> +++ b/fs/reiserfs/journal.c
> @@ -2591,12 +2591,7 @@ static void release_journal_dev(struct super_block *super,
>  			       struct reiserfs_journal *journal)
>  {
>  	if (journal->j_dev_bd != NULL) {
> -		void *holder = NULL;
> -
> -		if (journal->j_dev_bd->bd_dev != super->s_dev)
> -			holder = journal;
> -
> -		blkdev_put(journal->j_dev_bd, holder);
> +		blkdev_put(journal->j_dev_bd, super);
>  		journal->j_dev_bd = NULL;
>  	}
>  }
> @@ -2606,7 +2601,6 @@ static int journal_init_dev(struct super_block *super,
>  			    const char *jdev_name)
>  {
>  	blk_mode_t blkdev_mode = BLK_OPEN_READ;
> -	void *holder = journal;
>  	int result;
>  	dev_t jdev;
>  
> @@ -2621,10 +2615,8 @@ static int journal_init_dev(struct super_block *super,
>  
>  	/* there is no "jdev" option and journal is on separate device */
>  	if ((!jdev_name || !jdev_name[0])) {
> -		if (jdev == super->s_dev)
> -			holder = NULL;
> -		journal->j_dev_bd = blkdev_get_by_dev(jdev, blkdev_mode, holder,
> -						      NULL);
> +		journal->j_dev_bd = blkdev_get_by_dev(jdev, blkdev_mode, super,
> +						      &fs_holder_ops);
>  		if (IS_ERR(journal->j_dev_bd)) {
>  			result = PTR_ERR(journal->j_dev_bd);
>  			journal->j_dev_bd = NULL;
> @@ -2638,8 +2630,8 @@ static int journal_init_dev(struct super_block *super,
>  		return 0;
>  	}
>  
> -	journal->j_dev_bd = blkdev_get_by_path(jdev_name, blkdev_mode, holder,
> -					       NULL);
> +	journal->j_dev_bd = blkdev_get_by_path(jdev_name, blkdev_mode, super,
> +					       &fs_holder_ops);
>  	if (IS_ERR(journal->j_dev_bd)) {
>  		result = PTR_ERR(journal->j_dev_bd);
>  		journal->j_dev_bd = NULL;
> 
> -- 
> 2.34.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 2/4] reiserfs: centralize freeing of reiserfs info

[Christian Brauner]

Currently the reiserfs info is free in multiple locations:

* in reiserfs_fill_super() if reiserfs_fill_super() fails
* in reiserfs_put_super() when reiserfs is shut down and
  reiserfs_fill_super() had succeeded

Stop duplicating this logic and always free reiserfs info in
reiserfs_kill_sb().

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/reiserfs/super.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/fs/reiserfs/super.c b/fs/reiserfs/super.c
index 7eaf36b3de12..6db8ed10a78d 100644
--- a/fs/reiserfs/super.c
+++ b/fs/reiserfs/super.c
@@ -549,7 +549,9 @@ int remove_save_link(struct inode *inode, int truncate)
 
 static void reiserfs_kill_sb(struct super_block *s)
 {
-	if (REISERFS_SB(s)) {
+	struct reiserfs_sb_info *sbi = REISERFS_SB(s);
+
+	if (sbi) {
 		reiserfs_proc_info_done(s);
 		/*
 		 * Force any pending inode evictions to occur now. Any
@@ -561,13 +563,16 @@ static void reiserfs_kill_sb(struct super_block *s)
 		 */
 		shrink_dcache_sb(s);
 
-		dput(REISERFS_SB(s)->xattr_root);
-		REISERFS_SB(s)->xattr_root = NULL;
-		dput(REISERFS_SB(s)->priv_root);
-		REISERFS_SB(s)->priv_root = NULL;
+		dput(sbi->xattr_root);
+		sbi->xattr_root = NULL;
+		dput(sbi->priv_root);
+		sbi->priv_root = NULL;
 	}
 
 	kill_block_super(s);
+
+	kfree(sbi);
+	s->s_fs_info = NULL;
 }
 
 #ifdef CONFIG_QUOTA
@@ -630,8 +635,6 @@ static void reiserfs_put_super(struct super_block *s)
 	mutex_destroy(&REISERFS_SB(s)->lock);
 	destroy_workqueue(REISERFS_SB(s)->commit_wq);
 	kfree(REISERFS_SB(s)->s_jdev);
-	kfree(s->s_fs_info);
-	s->s_fs_info = NULL;
 }
 
 static struct kmem_cache *reiserfs_inode_cachep;
@@ -2240,9 +2243,6 @@ static int reiserfs_fill_super(struct super_block *s, void *data, int silent)
 	}
 #endif
 	kfree(sbi->s_jdev);
-	kfree(sbi);
-
-	s->s_fs_info = NULL;
 	return errval;
 }
 

-- 
2.34.1

Re: [PATCH 2/4] reiserfs: centralize freeing of reiserfs info

[Jan Kara]

On Mon 09-10-23 14:33:39, Christian Brauner wrote:
> Currently the reiserfs info is free in multiple locations:
> 
> * in reiserfs_fill_super() if reiserfs_fill_super() fails
> * in reiserfs_put_super() when reiserfs is shut down and
>   reiserfs_fill_super() had succeeded
> 
> Stop duplicating this logic and always free reiserfs info in
> reiserfs_kill_sb().
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/reiserfs/super.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/reiserfs/super.c b/fs/reiserfs/super.c
> index 7eaf36b3de12..6db8ed10a78d 100644
> --- a/fs/reiserfs/super.c
> +++ b/fs/reiserfs/super.c
> @@ -549,7 +549,9 @@ int remove_save_link(struct inode *inode, int truncate)
>  
>  static void reiserfs_kill_sb(struct super_block *s)
>  {
> -	if (REISERFS_SB(s)) {
> +	struct reiserfs_sb_info *sbi = REISERFS_SB(s);
> +
> +	if (sbi) {
>  		reiserfs_proc_info_done(s);
>  		/*
>  		 * Force any pending inode evictions to occur now. Any
> @@ -561,13 +563,16 @@ static void reiserfs_kill_sb(struct super_block *s)
>  		 */
>  		shrink_dcache_sb(s);
>  
> -		dput(REISERFS_SB(s)->xattr_root);
> -		REISERFS_SB(s)->xattr_root = NULL;
> -		dput(REISERFS_SB(s)->priv_root);
> -		REISERFS_SB(s)->priv_root = NULL;
> +		dput(sbi->xattr_root);
> +		sbi->xattr_root = NULL;
> +		dput(sbi->priv_root);
> +		sbi->priv_root = NULL;
>  	}
>  
>  	kill_block_super(s);
> +
> +	kfree(sbi);
> +	s->s_fs_info = NULL;
>  }
>  
>  #ifdef CONFIG_QUOTA
> @@ -630,8 +635,6 @@ static void reiserfs_put_super(struct super_block *s)
>  	mutex_destroy(&REISERFS_SB(s)->lock);
>  	destroy_workqueue(REISERFS_SB(s)->commit_wq);
>  	kfree(REISERFS_SB(s)->s_jdev);
> -	kfree(s->s_fs_info);
> -	s->s_fs_info = NULL;
>  }
>  
>  static struct kmem_cache *reiserfs_inode_cachep;
> @@ -2240,9 +2243,6 @@ static int reiserfs_fill_super(struct super_block *s, void *data, int silent)
>  	}
>  #endif
>  	kfree(sbi->s_jdev);
> -	kfree(sbi);
> -
> -	s->s_fs_info = NULL;
>  	return errval;
>  }
>  
> 
> -- 
> 2.34.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 3/4] reiserfs: centralize journal device closing

[Christian Brauner]

Currently the journal device is closed in multiple locations:

* in reiserfs_fill_super() if reiserfs_fill_super() fails
* in reiserfs_put_super() when reiserfs is shut down and
  reiserfs_fill_super() had succeeded

Stop duplicating this logic and always kill the journal device in
reiserfs_kill_b().

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/reiserfs/journal.c  | 18 ++++++++----------
 fs/reiserfs/reiserfs.h |  2 ++
 fs/reiserfs/super.c    |  4 ++++
 3 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/fs/reiserfs/journal.c b/fs/reiserfs/journal.c
index b9d9bf26d108..e001a96fc76c 100644
--- a/fs/reiserfs/journal.c
+++ b/fs/reiserfs/journal.c
@@ -90,8 +90,6 @@ static int flush_commit_list(struct super_block *s,
 static int can_dirty(struct reiserfs_journal_cnode *cn);
 static int journal_join(struct reiserfs_transaction_handle *th,
 			struct super_block *sb);
-static void release_journal_dev(struct super_block *super,
-			       struct reiserfs_journal *journal);
 static void dirty_one_transaction(struct super_block *s,
 				 struct reiserfs_journal_list *jl);
 static void flush_async_commits(struct work_struct *work);
@@ -1889,12 +1887,6 @@ static void free_journal_ram(struct super_block *sb)
 	if (journal->j_header_bh) {
 		brelse(journal->j_header_bh);
 	}
-	/*
-	 * j_header_bh is on the journal dev, make sure
-	 * not to release the journal dev until we brelse j_header_bh
-	 */
-	release_journal_dev(sb, journal);
-	vfree(journal);
 }
 
 /*
@@ -2587,13 +2579,19 @@ static void journal_list_init(struct super_block *sb)
 	SB_JOURNAL(sb)->j_current_jl = alloc_journal_list(sb);
 }
 
-static void release_journal_dev(struct super_block *super,
-			       struct reiserfs_journal *journal)
+void reiserfs_release_journal_dev(struct super_block *super,
+				  struct reiserfs_journal *journal)
 {
 	if (journal->j_dev_bd != NULL) {
 		blkdev_put(journal->j_dev_bd, super);
 		journal->j_dev_bd = NULL;
 	}
+
+	/*
+	 * j_header_bh is on the journal dev, make sure not to release
+	 * the journal dev until we brelse j_header_bh
+	 */
+	vfree(journal);
 }
 
 static int journal_init_dev(struct super_block *super,
diff --git a/fs/reiserfs/reiserfs.h b/fs/reiserfs/reiserfs.h
index 7d12b8c5b2fa..dd5d69c25e32 100644
--- a/fs/reiserfs/reiserfs.h
+++ b/fs/reiserfs/reiserfs.h
@@ -3414,3 +3414,5 @@ long reiserfs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg);
 long reiserfs_compat_ioctl(struct file *filp,
 		   unsigned int cmd, unsigned long arg);
 int reiserfs_unpack(struct inode *inode);
+void reiserfs_release_journal_dev(struct super_block *super,
+				  struct reiserfs_journal *journal);
diff --git a/fs/reiserfs/super.c b/fs/reiserfs/super.c
index 6db8ed10a78d..c04d9a4427e5 100644
--- a/fs/reiserfs/super.c
+++ b/fs/reiserfs/super.c
@@ -550,6 +550,7 @@ int remove_save_link(struct inode *inode, int truncate)
 static void reiserfs_kill_sb(struct super_block *s)
 {
 	struct reiserfs_sb_info *sbi = REISERFS_SB(s);
+	struct reiserfs_journal *journal = NULL;
 
 	if (sbi) {
 		reiserfs_proc_info_done(s);
@@ -567,10 +568,13 @@ static void reiserfs_kill_sb(struct super_block *s)
 		sbi->xattr_root = NULL;
 		dput(sbi->priv_root);
 		sbi->priv_root = NULL;
+		journal = SB_JOURNAL(s);
 	}
 
 	kill_block_super(s);
 
+	if (journal)
+		reiserfs_release_journal_dev(s, journal);
 	kfree(sbi);
 	s->s_fs_info = NULL;
 }

-- 
2.34.1

Re: [PATCH 3/4] reiserfs: centralize journal device closing

[Jan Kara]

On Mon 09-10-23 14:33:40, Christian Brauner wrote:
> Currently the journal device is closed in multiple locations:
> 
> * in reiserfs_fill_super() if reiserfs_fill_super() fails
> * in reiserfs_put_super() when reiserfs is shut down and
>   reiserfs_fill_super() had succeeded
> 
> Stop duplicating this logic and always kill the journal device in
> reiserfs_kill_b().
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/reiserfs/journal.c  | 18 ++++++++----------
>  fs/reiserfs/reiserfs.h |  2 ++
>  fs/reiserfs/super.c    |  4 ++++
>  3 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/reiserfs/journal.c b/fs/reiserfs/journal.c
> index b9d9bf26d108..e001a96fc76c 100644
> --- a/fs/reiserfs/journal.c
> +++ b/fs/reiserfs/journal.c
> @@ -90,8 +90,6 @@ static int flush_commit_list(struct super_block *s,
>  static int can_dirty(struct reiserfs_journal_cnode *cn);
>  static int journal_join(struct reiserfs_transaction_handle *th,
>  			struct super_block *sb);
> -static void release_journal_dev(struct super_block *super,
> -			       struct reiserfs_journal *journal);
>  static void dirty_one_transaction(struct super_block *s,
>  				 struct reiserfs_journal_list *jl);
>  static void flush_async_commits(struct work_struct *work);
> @@ -1889,12 +1887,6 @@ static void free_journal_ram(struct super_block *sb)
>  	if (journal->j_header_bh) {
>  		brelse(journal->j_header_bh);
>  	}
> -	/*
> -	 * j_header_bh is on the journal dev, make sure
> -	 * not to release the journal dev until we brelse j_header_bh
> -	 */
> -	release_journal_dev(sb, journal);
> -	vfree(journal);
>  }
>  
>  /*
> @@ -2587,13 +2579,19 @@ static void journal_list_init(struct super_block *sb)
>  	SB_JOURNAL(sb)->j_current_jl = alloc_journal_list(sb);
>  }
>  
> -static void release_journal_dev(struct super_block *super,
> -			       struct reiserfs_journal *journal)
> +void reiserfs_release_journal_dev(struct super_block *super,
> +				  struct reiserfs_journal *journal)
>  {
>  	if (journal->j_dev_bd != NULL) {
>  		blkdev_put(journal->j_dev_bd, super);
>  		journal->j_dev_bd = NULL;
>  	}
> +
> +	/*
> +	 * j_header_bh is on the journal dev, make sure not to release
> +	 * the journal dev until we brelse j_header_bh
> +	 */
> +	vfree(journal);
>  }
>  
>  static int journal_init_dev(struct super_block *super,
> diff --git a/fs/reiserfs/reiserfs.h b/fs/reiserfs/reiserfs.h
> index 7d12b8c5b2fa..dd5d69c25e32 100644
> --- a/fs/reiserfs/reiserfs.h
> +++ b/fs/reiserfs/reiserfs.h
> @@ -3414,3 +3414,5 @@ long reiserfs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg);
>  long reiserfs_compat_ioctl(struct file *filp,
>  		   unsigned int cmd, unsigned long arg);
>  int reiserfs_unpack(struct inode *inode);
> +void reiserfs_release_journal_dev(struct super_block *super,
> +				  struct reiserfs_journal *journal);
> diff --git a/fs/reiserfs/super.c b/fs/reiserfs/super.c
> index 6db8ed10a78d..c04d9a4427e5 100644
> --- a/fs/reiserfs/super.c
> +++ b/fs/reiserfs/super.c
> @@ -550,6 +550,7 @@ int remove_save_link(struct inode *inode, int truncate)
>  static void reiserfs_kill_sb(struct super_block *s)
>  {
>  	struct reiserfs_sb_info *sbi = REISERFS_SB(s);
> +	struct reiserfs_journal *journal = NULL;
>  
>  	if (sbi) {
>  		reiserfs_proc_info_done(s);
> @@ -567,10 +568,13 @@ static void reiserfs_kill_sb(struct super_block *s)
>  		sbi->xattr_root = NULL;
>  		dput(sbi->priv_root);
>  		sbi->priv_root = NULL;
> +		journal = SB_JOURNAL(s);
>  	}
>  
>  	kill_block_super(s);
>  
> +	if (journal)
> +		reiserfs_release_journal_dev(s, journal);
>  	kfree(sbi);
>  	s->s_fs_info = NULL;
>  }
> 
> -- 
> 2.34.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 4/4] reiserfs: fix journal device opening

[Christian Brauner]

We can't open devices with s_umount held without risking deadlocks.
So drop s_umount and reacquire it when opening the journal device.

Reported-by: syzbot+062317ea1d0a6d5e29e7@syzkaller.appspotmail.com
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/reiserfs/journal.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/fs/reiserfs/journal.c b/fs/reiserfs/journal.c
index e001a96fc76c..0c680de72d43 100644
--- a/fs/reiserfs/journal.c
+++ b/fs/reiserfs/journal.c
@@ -2714,7 +2714,7 @@ int journal_init(struct super_block *sb, const char *j_dev_name,
 	struct reiserfs_journal_header *jh;
 	struct reiserfs_journal *journal;
 	struct reiserfs_journal_list *jl;
-	int ret;
+	int ret = 1;
 
 	journal = SB_JOURNAL(sb) = vzalloc(sizeof(struct reiserfs_journal));
 	if (!journal) {
@@ -2727,6 +2727,13 @@ int journal_init(struct super_block *sb, const char *j_dev_name,
 	INIT_LIST_HEAD(&journal->j_working_list);
 	INIT_LIST_HEAD(&journal->j_journal_list);
 	journal->j_persistent_trans = 0;
+
+	/*
+	 * blkdev_put() can't be called under s_umount, see the comment
+	 * in get_tree_bdev() for more details
+	 */
+	up_write(&sb->s_umount);
+
 	if (reiserfs_allocate_list_bitmaps(sb, journal->j_list_bitmap,
 					   reiserfs_bmap_count(sb)))
 		goto free_and_return;
@@ -2891,8 +2898,7 @@ int journal_init(struct super_block *sb, const char *j_dev_name,
 		goto free_and_return;
 	}
 
-	ret = journal_read(sb);
-	if (ret < 0) {
+	if (journal_read(sb) < 0) {
 		reiserfs_warning(sb, "reiserfs-2006",
 				 "Replay Failure, unable to mount");
 		goto free_and_return;
@@ -2900,10 +2906,14 @@ int journal_init(struct super_block *sb, const char *j_dev_name,
 
 	INIT_DELAYED_WORK(&journal->j_work, flush_async_commits);
 	journal->j_work_sb = sb;
-	return 0;
+	ret = 0;
+
 free_and_return:
-	free_journal_ram(sb);
-	return 1;
+	if (ret)
+		free_journal_ram(sb);
+
+	down_write(&sb->s_umount);
+	return ret;
 }
 
 /*

-- 
2.34.1

Re: [PATCH 4/4] reiserfs: fix journal device opening

[Jan Kara]

On Mon 09-10-23 14:33:41, Christian Brauner wrote:
> We can't open devices with s_umount held without risking deadlocks.
> So drop s_umount and reacquire it when opening the journal device.
> 
> Reported-by: syzbot+062317ea1d0a6d5e29e7@syzkaller.appspotmail.com
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/reiserfs/journal.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/reiserfs/journal.c b/fs/reiserfs/journal.c
> index e001a96fc76c..0c680de72d43 100644
> --- a/fs/reiserfs/journal.c
> +++ b/fs/reiserfs/journal.c
> @@ -2714,7 +2714,7 @@ int journal_init(struct super_block *sb, const char *j_dev_name,
>  	struct reiserfs_journal_header *jh;
>  	struct reiserfs_journal *journal;
>  	struct reiserfs_journal_list *jl;
> -	int ret;
> +	int ret = 1;
>  
>  	journal = SB_JOURNAL(sb) = vzalloc(sizeof(struct reiserfs_journal));
>  	if (!journal) {
> @@ -2727,6 +2727,13 @@ int journal_init(struct super_block *sb, const char *j_dev_name,
>  	INIT_LIST_HEAD(&journal->j_working_list);
>  	INIT_LIST_HEAD(&journal->j_journal_list);
>  	journal->j_persistent_trans = 0;
> +
> +	/*
> +	 * blkdev_put() can't be called under s_umount, see the comment
> +	 * in get_tree_bdev() for more details
> +	 */
> +	up_write(&sb->s_umount);
> +
>  	if (reiserfs_allocate_list_bitmaps(sb, journal->j_list_bitmap,
>  					   reiserfs_bmap_count(sb)))
>  		goto free_and_return;
> @@ -2891,8 +2898,7 @@ int journal_init(struct super_block *sb, const char *j_dev_name,
>  		goto free_and_return;
>  	}
>  
> -	ret = journal_read(sb);
> -	if (ret < 0) {
> +	if (journal_read(sb) < 0) {
>  		reiserfs_warning(sb, "reiserfs-2006",
>  				 "Replay Failure, unable to mount");
>  		goto free_and_return;
> @@ -2900,10 +2906,14 @@ int journal_init(struct super_block *sb, const char *j_dev_name,
>  
>  	INIT_DELAYED_WORK(&journal->j_work, flush_async_commits);
>  	journal->j_work_sb = sb;
> -	return 0;
> +	ret = 0;
> +
>  free_and_return:
> -	free_journal_ram(sb);
> -	return 1;
> +	if (ret)
> +		free_journal_ram(sb);
> +
> +	down_write(&sb->s_umount);
> +	return ret;
>  }
>  
>  /*
> 
> -- 
> 2.34.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 4/4] reiserfs: fix journal device opening

[Christian Brauner]

On Mon, Oct 09, 2023 at 02:33:41PM +0200, Christian Brauner wrote:
> We can't open devices with s_umount held without risking deadlocks.
> So drop s_umount and reacquire it when opening the journal device.
> 
> Reported-by: syzbot+062317ea1d0a6d5e29e7@syzkaller.appspotmail.com
> Signed-off-by: Christian Brauner <brauner@kernel.org>
> ---

Groan, I added a dumb bug in here.

Re: [PATCH 4/4] reiserfs: fix journal device opening

[Jan Kara]

On Mon 09-10-23 18:16:45, Christian Brauner wrote:
> On Mon, Oct 09, 2023 at 02:33:41PM +0200, Christian Brauner wrote:
> > We can't open devices with s_umount held without risking deadlocks.
> > So drop s_umount and reacquire it when opening the journal device.
> > 
> > Reported-by: syzbot+062317ea1d0a6d5e29e7@syzkaller.appspotmail.com
> > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > ---
> 
> Groan, I added a dumb bug in here.

Which one? I went through the patch again but I still don't see it...

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 4/4] reiserfs: fix journal device opening

[Jan Kara]

On Tue 10-10-23 11:43:59, Christian Brauner wrote:
> On Mon, Oct 9, 2023, 18:34 Jan Kara <jack@suse.cz> wrote:
> 
> > On Mon 09-10-23 18:16:45, Christian Brauner wrote:
> > > On Mon, Oct 09, 2023 at 02:33:41PM +0200, Christian Brauner wrote:
> > > > We can't open devices with s_umount held without risking deadlocks.
> > > > So drop s_umount and reacquire it when opening the journal device.
> > > >
> > > > Reported-by: syzbot+062317ea1d0a6d5e29e7@syzkaller.appspotmail.com
> > > > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > > > ---
> > >
> > > Groan, I added a dumb bug in here.
> >
> > Which one? I went through the patch again but I still don't see it...
> >
> 
> (Sorry, from phone.)
> 
> I'm dropping s_umount across a lot of work
> instead of just over device opening which is really the wrong way of doing
> this.
> I should just drop it over journal_dev_init().

So I was kind of suspecting that but I couldn't figure out how it would
exactly matter when SB_BORN still is not set...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR