| Message ID | 964a8b27-cd69-357c-fe78-76b066056201@I-love.SAKURA.ne.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, May 15, 2018 at 07:11:06PM +0900, Tetsuo Handa wrote: > From ffd64dcf946502e7bb1d23c021ee9a4fc92f9312 Mon Sep 17 00:00:00 2001 > From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > Date: Tue, 15 May 2018 12:23:03 +0900 > Subject: [PATCH] hfsplus: stop workqueue when fill_super() failed > > syzbot is reporting ODEBUG messages at hfsplus_fill_super() [1]. > This is because hfsplus_fill_super() forgot to call > cancel_delayed_work_sync(). > > As far as I can see, it is hfsplus_mark_mdb_dirty() from > hfsplus_new_inode() in hfsplus_fill_super() that calls > queue_delayed_work(). Therefore, I assume that hfsplus_new_inode() does not > fail if queue_delayed_work() was called, and the out_put_hidden_dir label > is the appropriate location to call cancel_delayed_work_sync(). > > [1] https://syzkaller.appspot.com/bug?id=a66f45e96fdbeb76b796bf46eb25ea878c42a6c9 > > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > Reported-by: syzbot <syzbot+4f2e5f086147d543ab03@syzkaller.appspotmail.com> > Cc: Al Viro <viro@zeniv.linux.org.uk> > --- > fs/hfsplus/super.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c > index 9e690ae..80abba5 100644 > --- a/fs/hfsplus/super.c > +++ b/fs/hfsplus/super.c > @@ -590,6 +590,7 @@ static int hfsplus_fill_super(struct super_block *sb, void *data, int silent) > return 0; > > out_put_hidden_dir: > + cancel_delayed_work_sync(&sbi->sync_work); > iput(sbi->hidden_dir); > out_put_root: > dput(sb->s_root); > -- > 1.8.3.1 > > I sent this same patch a couple of weeks ago: https://www.spinics.net/lists/linux-fsdevel/msg125240.html I should probably have sent it in reply to syzbot to prevent this kind of duplication of work. Sorry about that.
On Tue, 15 May 2018 19:11:06 +0900 Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote: > >From ffd64dcf946502e7bb1d23c021ee9a4fc92f9312 Mon Sep 17 00:00:00 2001 > From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > Date: Tue, 15 May 2018 12:23:03 +0900 > Subject: [PATCH] hfsplus: stop workqueue when fill_super() failed > > syzbot is reporting ODEBUG messages at hfsplus_fill_super() [1]. > This is because hfsplus_fill_super() forgot to call > cancel_delayed_work_sync(). > > As far as I can see, it is hfsplus_mark_mdb_dirty() from > hfsplus_new_inode() in hfsplus_fill_super() that calls > queue_delayed_work(). Therefore, I assume that hfsplus_new_inode() does not > fail if queue_delayed_work() was called, and the out_put_hidden_dir label > is the appropriate location to call cancel_delayed_work_sync(). Yes, I was scratching my head over that - it is quite unobvious whereabouts in hfsplus_fill_super() that the work actually starts getting scheduled. "somewhere after the last goto out_put_root" might be true, for now. But it isn't at all obvious and it isn't very maintainable. Perhaps it's simply wrong for hfsplus to be marking things dirty and performing these complex operations partway through fill_super() before everything is fully set up. And I wouldn't be comfortable putting the cancel_work_sync() right at the end of the hfsplus_fill_super() cleanup because the delayed work handler might be using things which have already been torn down by that stage. So... ugh. A solid shotgun approach would be to put a cancel_work_sync() immediately after each and every goto target in that cleanup path, but that's just stupid :( Nasty. I can't think of anything clever here. I guess we can go with this patch for now, and if new problems crop up we can look at moving the cancel_work_sync() down to a later part of the teardown sequence.
diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c index 9e690ae..80abba5 100644 --- a/fs/hfsplus/super.c +++ b/fs/hfsplus/super.c @@ -590,6 +590,7 @@ static int hfsplus_fill_super(struct super_block *sb, void *data, int silent) return 0; out_put_hidden_dir: + cancel_delayed_work_sync(&sbi->sync_work); iput(sbi->hidden_dir); out_put_root: dput(sb->s_root);