| Message ID | 20231009-vfs-fixes-reiserfs-v1-4-723a2f1132ce@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | reiserfs: fixes | expand |
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 >
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.
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
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
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; } /*
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(-)