Re: WARNING in kill_block_super
Al Viro wrote: > On Wed, Apr 11, 2018 at 10:28:06AM +0900, Tetsuo Handa wrote: > > Al Viro wrote: > > > On Wed, Apr 04, 2018 at 07:53:07PM +0900, Tetsuo Handa wrote: > > > > Al and Michal, are you OK with this patch? > > > > > > First of all, it does *NOT* fix the problems with careless ->kill_sb(). > > > The fuse-blk case is the only real rationale so far. Said that, > > > > > > > Please notice below one as well. Fixing all careless ->kill_sb() will be too > > difficult to backport. For now, avoid calling deactivate_locked_super() is > > safer. > > How will that fix e.g. jffs2? You can send patches which my patch does not fix. > > > [upstream] WARNING: refcount bug in put_pid_ns > > https://syzkaller.appspot.com/bug?id=17e202b4794da213570ba33ac2f70277ef1ce015 > > Should be fixed by 8e666cb33597 in that series, AFAICS. OK. Al Viro wrote: > On Wed, Apr 04, 2018 at 07:53:07PM +0900, Tetsuo Handa wrote: > > Al and Michal, are you OK with this patch? > > First of all, it does *NOT* fix the problems with careless ->kill_sb(). > The fuse-blk case is the only real rationale so far. Said that, > > > @@ -166,6 +166,7 @@ static void destroy_unused_super(struct super_block *s) > > security_sb_free(s); > > put_user_ns(s->s_user_ns); > > kfree(s->s_subtype); > > + kfree(s->s_shrink.nr_deferred); > > is probably better done with an inlined helper (fs/super.c has no business > knowing > about ->nr_deferred name, and there probably will be other users of that > preallocation of yours). And the same helper would be better off zeroing the > pointer, same as unregister_shrinker() does. > > > > -int register_shrinker(struct shrinker *shrinker) > > +int prepare_shrinker(struct shrinker *shrinker) > > preallocate_shrinker(), perhaps? > > > +int register_shrinker(struct shrinker *shrinker) > > +{ > > + int err = prepare_shrinker(shrinker); > > + > > + if (err) > > + return err; > > + register_shrinker_prepared(shrinker); > > if (!err) > register_; > return err; > > would be better, IMO. > OK. Here is version 2. What do you think? >From 9d035f5bee3861cb73c4d323c03121f431edf760 Mon Sep 17 00:00:00 2001 From: Tetsuo Handa <penguin-ker...@i-love.sakura.ne.jp> Date: Wed, 11 Apr 2018 11:44:46 +0900 Subject: [PATCH v2] mm,vmscan: Allow preallocating memory for register_shrinker(). syzbot is catching so many bugs triggered by commit 9ee332d99e4d5a97 ("sget(): handle failures of register_shrinker()"). That commit expected that calling kill_sb() from deactivate_locked_super() without successful fill_super() is safe. But it turned out that there are many bugs which exist before that commit, and also that that commit caused regressions in several cases. For example, [1] is a report where sb->s_mode (which seems to be either FMODE_READ | FMODE_EXCL | FMODE_WRITE or FMODE_READ | FMODE_EXCL) is not assigned unless sget() succeeds. But it does not worth complicate sget() so that register_shrinker() failure path can safely call kill_block_super() via kill_sb(). Making alloc_super() fail if memory allocation for register_shrinker() failed is much simpler. Although this patch hides some of bugs revealed by that commit, this patch allows preallocating memory for the shrinker. By this change, we can avoid calling deactivate_locked_super() from sget_userns(). Manual auditing and syzbot tests will eventually reveal remaining bugs. [1] https://syzkaller.appspot.com/bug?id=588996a25a2587be2e3a54e8646728fb9cae44e7 Signed-off-by: Tetsuo Handa <penguin-ker...@i-love.sakura.ne.jp> Reported-by: syzbot <syzbot+84371b6062cb639d7...@syzkaller.appspotmail.com> # WARNING: refcount bug in should_fail Reported-by: syzbot <syzbot+5a170e19c963a2e0d...@syzkaller.appspotmail.com> # WARNING in kill_block_super Reported-by: syzbot <syzbot+66a731f39da94bb14...@syzkaller.appspotmail.com> # WARNING: refcount bug in put_pid_ns Reported-by: syzbot <syzbot+7a1cff37dbbef9e7b...@syzkaller.appspotmail.com> # KASAN: use-after-free Read in alloc_pid Reported-by: syzbot <syzbot+151de3f2be6b40ac8...@syzkaller.appspotmail.com> # general protection fault in kernfs_kill_sb Cc: stable <sta...@vger.kernel.org> # 4.15+ Cc: Al Viro <v...@zeniv.linux.org.uk> Cc: Michal Hocko <mho...@suse.com> --- fs/super.c | 9 - include/linux/shrinker.h | 21 +++-- mm/vmscan.c | 20 +++- 3 files changed, 38 insertions(+), 12 deletions(-) diff --git a/fs/super.c b/fs/super.c index 672538c..5a839cd8 100644 --- a/fs/super.c +++ b/fs/super.c @@ -166,6 +166,7 @@ stat
Re: WARNING in kill_block_super
Al Viro wrote: > On Wed, Apr 11, 2018 at 10:28:06AM +0900, Tetsuo Handa wrote: > > Al Viro wrote: > > > On Wed, Apr 04, 2018 at 07:53:07PM +0900, Tetsuo Handa wrote: > > > > Al and Michal, are you OK with this patch? > > > > > > First of all, it does *NOT* fix the problems with careless ->kill_sb(). > > > The fuse-blk case is the only real rationale so far. Said that, > > > > > > > Please notice below one as well. Fixing all careless ->kill_sb() will be too > > difficult to backport. For now, avoid calling deactivate_locked_super() is > > safer. > > How will that fix e.g. jffs2? You can send patches which my patch does not fix. > > > [upstream] WARNING: refcount bug in put_pid_ns > > https://syzkaller.appspot.com/bug?id=17e202b4794da213570ba33ac2f70277ef1ce015 > > Should be fixed by 8e666cb33597 in that series, AFAICS. OK. Al Viro wrote: > On Wed, Apr 04, 2018 at 07:53:07PM +0900, Tetsuo Handa wrote: > > Al and Michal, are you OK with this patch? > > First of all, it does *NOT* fix the problems with careless ->kill_sb(). > The fuse-blk case is the only real rationale so far. Said that, > > > @@ -166,6 +166,7 @@ static void destroy_unused_super(struct super_block *s) > > security_sb_free(s); > > put_user_ns(s->s_user_ns); > > kfree(s->s_subtype); > > + kfree(s->s_shrink.nr_deferred); > > is probably better done with an inlined helper (fs/super.c has no business > knowing > about ->nr_deferred name, and there probably will be other users of that > preallocation of yours). And the same helper would be better off zeroing the > pointer, same as unregister_shrinker() does. > > > > -int register_shrinker(struct shrinker *shrinker) > > +int prepare_shrinker(struct shrinker *shrinker) > > preallocate_shrinker(), perhaps? > > > +int register_shrinker(struct shrinker *shrinker) > > +{ > > + int err = prepare_shrinker(shrinker); > > + > > + if (err) > > + return err; > > + register_shrinker_prepared(shrinker); > > if (!err) > register_; > return err; > > would be better, IMO. > OK. Here is version 2. What do you think? >From 9d035f5bee3861cb73c4d323c03121f431edf760 Mon Sep 17 00:00:00 2001 From: Tetsuo Handa Date: Wed, 11 Apr 2018 11:44:46 +0900 Subject: [PATCH v2] mm,vmscan: Allow preallocating memory for register_shrinker(). syzbot is catching so many bugs triggered by commit 9ee332d99e4d5a97 ("sget(): handle failures of register_shrinker()"). That commit expected that calling kill_sb() from deactivate_locked_super() without successful fill_super() is safe. But it turned out that there are many bugs which exist before that commit, and also that that commit caused regressions in several cases. For example, [1] is a report where sb->s_mode (which seems to be either FMODE_READ | FMODE_EXCL | FMODE_WRITE or FMODE_READ | FMODE_EXCL) is not assigned unless sget() succeeds. But it does not worth complicate sget() so that register_shrinker() failure path can safely call kill_block_super() via kill_sb(). Making alloc_super() fail if memory allocation for register_shrinker() failed is much simpler. Although this patch hides some of bugs revealed by that commit, this patch allows preallocating memory for the shrinker. By this change, we can avoid calling deactivate_locked_super() from sget_userns(). Manual auditing and syzbot tests will eventually reveal remaining bugs. [1] https://syzkaller.appspot.com/bug?id=588996a25a2587be2e3a54e8646728fb9cae44e7 Signed-off-by: Tetsuo Handa Reported-by: syzbot # WARNING: refcount bug in should_fail Reported-by: syzbot # WARNING in kill_block_super Reported-by: syzbot # WARNING: refcount bug in put_pid_ns Reported-by: syzbot # KASAN: use-after-free Read in alloc_pid Reported-by: syzbot # general protection fault in kernfs_kill_sb Cc: stable # 4.15+ Cc: Al Viro Cc: Michal Hocko --- fs/super.c | 9 - include/linux/shrinker.h | 21 +++-- mm/vmscan.c | 20 +++- 3 files changed, 38 insertions(+), 12 deletions(-) diff --git a/fs/super.c b/fs/super.c index 672538c..5a839cd8 100644 --- a/fs/super.c +++ b/fs/super.c @@ -166,6 +166,7 @@ static void destroy_unused_super(struct super_block *s) security_sb_free(s); put_user_ns(s->s_user_ns); kfree(s->s_subtype); + unallocate_shrinker(>s_shrink); /* no delays needed */ destroy_super_work(>destroy_work); } @@ -251,6 +252,8 @@ static struct super_block *alloc_super(struct file_system_type *type, int flags, s->s_shrink.count_objects = super_cache_count; s->s_shrink.batch = 1024;
Re: Re: WARNING in kill_block_super
On Wed, Apr 11, 2018 at 10:28:06AM +0900, Tetsuo Handa wrote: > Al Viro wrote: > > On Wed, Apr 04, 2018 at 07:53:07PM +0900, Tetsuo Handa wrote: > > > Al and Michal, are you OK with this patch? > > > > First of all, it does *NOT* fix the problems with careless ->kill_sb(). > > The fuse-blk case is the only real rationale so far. Said that, > > > > Please notice below one as well. Fixing all careless ->kill_sb() will be too > difficult to backport. For now, avoid calling deactivate_locked_super() is > safer. How will that fix e.g. jffs2? > [upstream] WARNING: refcount bug in put_pid_ns > https://syzkaller.appspot.com/bug?id=17e202b4794da213570ba33ac2f70277ef1ce015 Should be fixed by 8e666cb33597 in that series, AFAICS.
Re: Re: WARNING in kill_block_super
On Wed, Apr 11, 2018 at 10:28:06AM +0900, Tetsuo Handa wrote: > Al Viro wrote: > > On Wed, Apr 04, 2018 at 07:53:07PM +0900, Tetsuo Handa wrote: > > > Al and Michal, are you OK with this patch? > > > > First of all, it does *NOT* fix the problems with careless ->kill_sb(). > > The fuse-blk case is the only real rationale so far. Said that, > > > > Please notice below one as well. Fixing all careless ->kill_sb() will be too > difficult to backport. For now, avoid calling deactivate_locked_super() is > safer. How will that fix e.g. jffs2? > [upstream] WARNING: refcount bug in put_pid_ns > https://syzkaller.appspot.com/bug?id=17e202b4794da213570ba33ac2f70277ef1ce015 Should be fixed by 8e666cb33597 in that series, AFAICS.
Re: WARNING in kill_block_super
On Wed, Apr 04, 2018 at 07:53:07PM +0900, Tetsuo Handa wrote: > Al and Michal, are you OK with this patch? First of all, it does *NOT* fix the problems with careless ->kill_sb(). The fuse-blk case is the only real rationale so far. Said that, > @@ -166,6 +166,7 @@ static void destroy_unused_super(struct super_block *s) > security_sb_free(s); > put_user_ns(s->s_user_ns); > kfree(s->s_subtype); > + kfree(s->s_shrink.nr_deferred); is probably better done with an inlined helper (fs/super.c has no business knowing about ->nr_deferred name, and there probably will be other users of that preallocation of yours). And the same helper would be better off zeroing the pointer, same as unregister_shrinker() does. > -int register_shrinker(struct shrinker *shrinker) > +int prepare_shrinker(struct shrinker *shrinker) preallocate_shrinker(), perhaps? > +int register_shrinker(struct shrinker *shrinker) > +{ > + int err = prepare_shrinker(shrinker); > + > + if (err) > + return err; > + register_shrinker_prepared(shrinker); if (!err) register_; return err; would be better, IMO.
Re: WARNING in kill_block_super
On Wed, Apr 04, 2018 at 07:53:07PM +0900, Tetsuo Handa wrote: > Al and Michal, are you OK with this patch? First of all, it does *NOT* fix the problems with careless ->kill_sb(). The fuse-blk case is the only real rationale so far. Said that, > @@ -166,6 +166,7 @@ static void destroy_unused_super(struct super_block *s) > security_sb_free(s); > put_user_ns(s->s_user_ns); > kfree(s->s_subtype); > + kfree(s->s_shrink.nr_deferred); is probably better done with an inlined helper (fs/super.c has no business knowing about ->nr_deferred name, and there probably will be other users of that preallocation of yours). And the same helper would be better off zeroing the pointer, same as unregister_shrinker() does. > -int register_shrinker(struct shrinker *shrinker) > +int prepare_shrinker(struct shrinker *shrinker) preallocate_shrinker(), perhaps? > +int register_shrinker(struct shrinker *shrinker) > +{ > + int err = prepare_shrinker(shrinker); > + > + if (err) > + return err; > + register_shrinker_prepared(shrinker); if (!err) register_; return err; would be better, IMO.
Re: WARNING in kill_block_super
Michal Hocko wrote: > On Wed 04-04-18 19:53:07, Tetsuo Handa wrote: > > Al and Michal, are you OK with this patch? > > Maybe I've misunderstood, but hasn't Al explained [1] that the > appropriate fix is in the fs code? > > [1] http://lkml.kernel.org/r/20180402143415.gc30...@zeniv.linux.org.uk Yes. But I wonder whether it worth complicating sget() only for handling kmalloc() failure. static struct file_system_type fuseblk_fs_type = { .owner = THIS_MODULE, .name = "fuseblk", .mount = fuse_mount_blk, .kill_sb= fuse_kill_sb_blk, .fs_flags = FS_REQUIRES_DEV | FS_HAS_SUBTYPE, }; static struct dentry *fuse_mount_blk(struct file_system_type *fs_type, int flags, const char *dev_name, void *raw_data) { return mount_bdev(fs_type, flags, dev_name, raw_data, fuse_fill_super) { fmode_t mode = FMODE_READ | FMODE_EXCL; if (!(flags & MS_RDONLY)) mode |= FMODE_WRITE; s = sget(fs_type, test_bdev_super, set_bdev_super, flags | MS_NOSEC, bdev) { return sget_userns(type, test, set, flags, user_ns, data) { s = alloc_super(type, (flags & ~MS_SUBMOUNT), user_ns); err = register_shrinker(>s_shrink); if (err) { deactivate_locked_super(s) { fs->kill_sb(s) = fuse_kill_sb_blk(s) { kill_block_super(sb) { struct block_device *bdev = sb->s_bdev; fmode_t mode = sb->s_mode; WARN_ON_ONCE(!(mode & FMODE_EXCL)); // <= Unsafe because FMODE_EXCL is not yet set which will be set at blkdev_put(bdev, mode | FMODE_EXCL); } } } s = ERR_PTR(err); } } } /* If sget() succeeds then ... */ s->s_mode = mode; // <= this location. error = fill_super(s, data, flags & MS_SILENT ? 1 : 0); if (error) { deactivate_locked_super(s) { fs->kill_sb(s) = fuse_kill_sb_blk(s) { kill_block_super(sb) { struct block_device *bdev = sb->s_bdev; fmode_t mode = sb->s_mode; WARN_ON_ONCE(!(mode & FMODE_EXCL)); // <= Safe because FMODE_EXCL already set. blkdev_put(bdev, mode | FMODE_EXCL); } } } goto error; } /* If sget() fails then ... */ error = PTR_ERR(s); blkdev_put(bdev, mode); // <= Calls blkdev_put() after deactivate_locked_super() already called blkdev_put(). } } mount_bdev() is not ready to call blkdev_put() from sget(). Do we want to pass "s->s_mode" to sget() which allocates "s" ? I feel it is preposterous that a function which allocates memory for an object requires some of fields being already initialized in order to call a destroy function. By splitting register_shrinker() into prepare_shrinker() which might fail and register_shrinker_prepared() which will not fail, we can allow shrinker users to allocate memory at object creation time. I wrote a patch which adds __must_check to register_shrinker() and we keep that patch in linux-next.git, but what we got is a fake change which do not implement proper error handling (e.g. Commit 6c4ca1e36cdc1a0a ("bcache: check return value of register_shrinker") if (register_shrinker(>shrink)) pr_warn("bcache: %s: could not register shrinker", __func__); ). It is not trivial to undo an error at register_shrinker(). Allocating memory for the shrinker at the time memory for an object which contains the shrinker is allocated is much easier to undo.
Re: WARNING in kill_block_super
Michal Hocko wrote: > On Wed 04-04-18 19:53:07, Tetsuo Handa wrote: > > Al and Michal, are you OK with this patch? > > Maybe I've misunderstood, but hasn't Al explained [1] that the > appropriate fix is in the fs code? > > [1] http://lkml.kernel.org/r/20180402143415.gc30...@zeniv.linux.org.uk Yes. But I wonder whether it worth complicating sget() only for handling kmalloc() failure. static struct file_system_type fuseblk_fs_type = { .owner = THIS_MODULE, .name = "fuseblk", .mount = fuse_mount_blk, .kill_sb= fuse_kill_sb_blk, .fs_flags = FS_REQUIRES_DEV | FS_HAS_SUBTYPE, }; static struct dentry *fuse_mount_blk(struct file_system_type *fs_type, int flags, const char *dev_name, void *raw_data) { return mount_bdev(fs_type, flags, dev_name, raw_data, fuse_fill_super) { fmode_t mode = FMODE_READ | FMODE_EXCL; if (!(flags & MS_RDONLY)) mode |= FMODE_WRITE; s = sget(fs_type, test_bdev_super, set_bdev_super, flags | MS_NOSEC, bdev) { return sget_userns(type, test, set, flags, user_ns, data) { s = alloc_super(type, (flags & ~MS_SUBMOUNT), user_ns); err = register_shrinker(>s_shrink); if (err) { deactivate_locked_super(s) { fs->kill_sb(s) = fuse_kill_sb_blk(s) { kill_block_super(sb) { struct block_device *bdev = sb->s_bdev; fmode_t mode = sb->s_mode; WARN_ON_ONCE(!(mode & FMODE_EXCL)); // <= Unsafe because FMODE_EXCL is not yet set which will be set at blkdev_put(bdev, mode | FMODE_EXCL); } } } s = ERR_PTR(err); } } } /* If sget() succeeds then ... */ s->s_mode = mode; // <= this location. error = fill_super(s, data, flags & MS_SILENT ? 1 : 0); if (error) { deactivate_locked_super(s) { fs->kill_sb(s) = fuse_kill_sb_blk(s) { kill_block_super(sb) { struct block_device *bdev = sb->s_bdev; fmode_t mode = sb->s_mode; WARN_ON_ONCE(!(mode & FMODE_EXCL)); // <= Safe because FMODE_EXCL already set. blkdev_put(bdev, mode | FMODE_EXCL); } } } goto error; } /* If sget() fails then ... */ error = PTR_ERR(s); blkdev_put(bdev, mode); // <= Calls blkdev_put() after deactivate_locked_super() already called blkdev_put(). } } mount_bdev() is not ready to call blkdev_put() from sget(). Do we want to pass "s->s_mode" to sget() which allocates "s" ? I feel it is preposterous that a function which allocates memory for an object requires some of fields being already initialized in order to call a destroy function. By splitting register_shrinker() into prepare_shrinker() which might fail and register_shrinker_prepared() which will not fail, we can allow shrinker users to allocate memory at object creation time. I wrote a patch which adds __must_check to register_shrinker() and we keep that patch in linux-next.git, but what we got is a fake change which do not implement proper error handling (e.g. Commit 6c4ca1e36cdc1a0a ("bcache: check return value of register_shrinker") if (register_shrinker(>shrink)) pr_warn("bcache: %s: could not register shrinker", __func__); ). It is not trivial to undo an error at register_shrinker(). Allocating memory for the shrinker at the time memory for an object which contains the shrinker is allocated is much easier to undo.
Re: WARNING in kill_block_super
On Wed 04-04-18 19:53:07, Tetsuo Handa wrote: > Al and Michal, are you OK with this patch? Maybe I've misunderstood, but hasn't Al explained [1] that the appropriate fix is in the fs code? [1] http://lkml.kernel.org/r/20180402143415.gc30...@zeniv.linux.org.uk -- Michal Hocko SUSE Labs
Re: WARNING in kill_block_super
On Wed 04-04-18 19:53:07, Tetsuo Handa wrote: > Al and Michal, are you OK with this patch? Maybe I've misunderstood, but hasn't Al explained [1] that the appropriate fix is in the fs code? [1] http://lkml.kernel.org/r/20180402143415.gc30...@zeniv.linux.org.uk -- Michal Hocko SUSE Labs
Re: WARNING in kill_block_super
Al and Michal, are you OK with this patch? >From bbc0d00935ebcb7e287403bae545fae9340830d9 Mon Sep 17 00:00:00 2001 From: Tetsuo HandaDate: Wed, 4 Apr 2018 12:19:42 +0900 Subject: [PATCH] mm,vmscan: Allow preallocating memory for register_shrinker(). syzbot is catching so many bugs triggered by commit 9ee332d99e4d5a97 ("sget(): handle failures of register_shrinker()"). That commit expected that calling kill_sb() from deactivate_locked_super() without successful fill_super() is safe, but the reality was different; some callers assign attributes which are needed for kill_sb() after sget() succeeds. For example, [1] is a report where sb->s_mode (which seems to be either FMODE_READ | FMODE_EXCL | FMODE_WRITE or FMODE_READ | FMODE_EXCL) is not assigned unless sget() succeeds. But it does not worth complicate sget() so that register_shrinker() failure path can safely call kill_block_super() via kill_sb(). Making alloc_super() fail if memory allocation for register_shrinker() failed is much simpler. Let's avoid calling deactivate_locked_super() from sget_userns() by preallocating memory for the shrinker and making register_shrinker() in sget_userns() never fail. [1] https://syzkaller.appspot.com/bug?id=588996a25a2587be2e3a54e8646728fb9cae44e7 Signed-off-by: Tetsuo Handa Reported-by: syzbot Cc: Al Viro Cc: Michal Hocko --- fs/super.c | 9 - include/linux/shrinker.h | 6 -- mm/vmscan.c | 15 ++- 3 files changed, 22 insertions(+), 8 deletions(-) diff --git a/fs/super.c b/fs/super.c index 672538c..db00f67 100644 --- a/fs/super.c +++ b/fs/super.c @@ -166,6 +166,7 @@ static void destroy_unused_super(struct super_block *s) security_sb_free(s); put_user_ns(s->s_user_ns); kfree(s->s_subtype); + kfree(s->s_shrink.nr_deferred); /* no delays needed */ destroy_super_work(>destroy_work); } @@ -251,6 +252,8 @@ static struct super_block *alloc_super(struct file_system_type *type, int flags, s->s_shrink.count_objects = super_cache_count; s->s_shrink.batch = 1024; s->s_shrink.flags = SHRINKER_NUMA_AWARE | SHRINKER_MEMCG_AWARE; + if (prepare_shrinker(>s_shrink)) + goto fail; return s; fail: @@ -517,11 +520,7 @@ struct super_block *sget_userns(struct file_system_type *type, hlist_add_head(>s_instances, >fs_supers); spin_unlock(_lock); get_filesystem(type); - err = register_shrinker(>s_shrink); - if (err) { - deactivate_locked_super(s); - s = ERR_PTR(err); - } + register_shrinker_prepared(>s_shrink); return s; } diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h index 388ff29..2728918 100644 --- a/include/linux/shrinker.h +++ b/include/linux/shrinker.h @@ -75,6 +75,8 @@ struct shrinker { #define SHRINKER_NUMA_AWARE(1 << 0) #define SHRINKER_MEMCG_AWARE (1 << 1) -extern int register_shrinker(struct shrinker *); -extern void unregister_shrinker(struct shrinker *); +extern int prepare_shrinker(struct shrinker *shrinker); +extern void register_shrinker_prepared(struct shrinker *shrinker); +extern int register_shrinker(struct shrinker *shrinker); +extern void unregister_shrinker(struct shrinker *shrinker); #endif diff --git a/mm/vmscan.c b/mm/vmscan.c index cd5dc3f..a10fe8e 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -258,7 +258,7 @@ unsigned long lruvec_lru_size(struct lruvec *lruvec, enum lru_list lru, int zone /* * Add a shrinker callback to be called from the vm. */ -int register_shrinker(struct shrinker *shrinker) +int prepare_shrinker(struct shrinker *shrinker) { size_t size = sizeof(*shrinker->nr_deferred); @@ -268,10 +268,23 @@ int register_shrinker(struct shrinker *shrinker) shrinker->nr_deferred = kzalloc(size, GFP_KERNEL); if (!shrinker->nr_deferred) return -ENOMEM; + return 0; +} +void register_shrinker_prepared(struct shrinker *shrinker) +{ down_write(_rwsem); list_add_tail(>list, _list); up_write(_rwsem); +} + +int register_shrinker(struct shrinker *shrinker) +{ + int err = prepare_shrinker(shrinker); + + if (err) + return err; + register_shrinker_prepared(shrinker); return 0; } EXPORT_SYMBOL(register_shrinker); -- 1.8.3.1
Re: WARNING in kill_block_super
Al and Michal, are you OK with this patch? >From bbc0d00935ebcb7e287403bae545fae9340830d9 Mon Sep 17 00:00:00 2001 From: Tetsuo Handa Date: Wed, 4 Apr 2018 12:19:42 +0900 Subject: [PATCH] mm,vmscan: Allow preallocating memory for register_shrinker(). syzbot is catching so many bugs triggered by commit 9ee332d99e4d5a97 ("sget(): handle failures of register_shrinker()"). That commit expected that calling kill_sb() from deactivate_locked_super() without successful fill_super() is safe, but the reality was different; some callers assign attributes which are needed for kill_sb() after sget() succeeds. For example, [1] is a report where sb->s_mode (which seems to be either FMODE_READ | FMODE_EXCL | FMODE_WRITE or FMODE_READ | FMODE_EXCL) is not assigned unless sget() succeeds. But it does not worth complicate sget() so that register_shrinker() failure path can safely call kill_block_super() via kill_sb(). Making alloc_super() fail if memory allocation for register_shrinker() failed is much simpler. Let's avoid calling deactivate_locked_super() from sget_userns() by preallocating memory for the shrinker and making register_shrinker() in sget_userns() never fail. [1] https://syzkaller.appspot.com/bug?id=588996a25a2587be2e3a54e8646728fb9cae44e7 Signed-off-by: Tetsuo Handa Reported-by: syzbot Cc: Al Viro Cc: Michal Hocko --- fs/super.c | 9 - include/linux/shrinker.h | 6 -- mm/vmscan.c | 15 ++- 3 files changed, 22 insertions(+), 8 deletions(-) diff --git a/fs/super.c b/fs/super.c index 672538c..db00f67 100644 --- a/fs/super.c +++ b/fs/super.c @@ -166,6 +166,7 @@ static void destroy_unused_super(struct super_block *s) security_sb_free(s); put_user_ns(s->s_user_ns); kfree(s->s_subtype); + kfree(s->s_shrink.nr_deferred); /* no delays needed */ destroy_super_work(>destroy_work); } @@ -251,6 +252,8 @@ static struct super_block *alloc_super(struct file_system_type *type, int flags, s->s_shrink.count_objects = super_cache_count; s->s_shrink.batch = 1024; s->s_shrink.flags = SHRINKER_NUMA_AWARE | SHRINKER_MEMCG_AWARE; + if (prepare_shrinker(>s_shrink)) + goto fail; return s; fail: @@ -517,11 +520,7 @@ struct super_block *sget_userns(struct file_system_type *type, hlist_add_head(>s_instances, >fs_supers); spin_unlock(_lock); get_filesystem(type); - err = register_shrinker(>s_shrink); - if (err) { - deactivate_locked_super(s); - s = ERR_PTR(err); - } + register_shrinker_prepared(>s_shrink); return s; } diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h index 388ff29..2728918 100644 --- a/include/linux/shrinker.h +++ b/include/linux/shrinker.h @@ -75,6 +75,8 @@ struct shrinker { #define SHRINKER_NUMA_AWARE(1 << 0) #define SHRINKER_MEMCG_AWARE (1 << 1) -extern int register_shrinker(struct shrinker *); -extern void unregister_shrinker(struct shrinker *); +extern int prepare_shrinker(struct shrinker *shrinker); +extern void register_shrinker_prepared(struct shrinker *shrinker); +extern int register_shrinker(struct shrinker *shrinker); +extern void unregister_shrinker(struct shrinker *shrinker); #endif diff --git a/mm/vmscan.c b/mm/vmscan.c index cd5dc3f..a10fe8e 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -258,7 +258,7 @@ unsigned long lruvec_lru_size(struct lruvec *lruvec, enum lru_list lru, int zone /* * Add a shrinker callback to be called from the vm. */ -int register_shrinker(struct shrinker *shrinker) +int prepare_shrinker(struct shrinker *shrinker) { size_t size = sizeof(*shrinker->nr_deferred); @@ -268,10 +268,23 @@ int register_shrinker(struct shrinker *shrinker) shrinker->nr_deferred = kzalloc(size, GFP_KERNEL); if (!shrinker->nr_deferred) return -ENOMEM; + return 0; +} +void register_shrinker_prepared(struct shrinker *shrinker) +{ down_write(_rwsem); list_add_tail(>list, _list); up_write(_rwsem); +} + +int register_shrinker(struct shrinker *shrinker) +{ + int err = prepare_shrinker(shrinker); + + if (err) + return err; + register_shrinker_prepared(shrinker); return 0; } EXPORT_SYMBOL(register_shrinker); -- 1.8.3.1
WARNING in kill_block_super
Hello, syzbot hit the following crash on upstream commit 0b412605ef5f5c64b31f19e2910b1d5eba9929c3 (Thu Mar 29 01:07:23 2018 +) Merge tag 'drm-fixes-for-v4.16-rc8' of git://people.freedesktop.org/~airlied/linux syzbot dashboard link: https://syzkaller.appspot.com/bug?extid=5a170e19c963a2e0df79 So far this crash happened 2 times on upstream. C reproducer: https://syzkaller.appspot.com/x/repro.c?id=6168466046320640 syzkaller reproducer: https://syzkaller.appspot.com/x/repro.syz?id=4851040260194304 Raw console output: https://syzkaller.appspot.com/x/log.txt?id=5936700618440704 Kernel config: https://syzkaller.appspot.com/x/.config?id=-8440362230543204781 compiler: gcc (GCC) 7.1.1 20170620 IMPORTANT: if you fix the bug, please add the following tag to the commit: Reported-by: syzbot+5a170e19c963a2e0d...@syzkaller.appspotmail.com It will help syzbot understand when the bug is fixed. See footer for details. If you forward the report, please keep this part and the footer. RAX: ffda RBX: 7ffed0bc62a0 RCX: 00442e79 RDX: 004a4d6d RSI: 20001740 RDI: 20001780 RBP: R08: 7ffed0bc62a0 R09: R10: R11: 0206 R12: R13: 0008 R14: 1380 R15: 7ffed0bc5b38 WARNING: CPU: 1 PID: 4445 at fs/super.c:1148 kill_block_super+0xc9/0xf0 fs/super.c:1148 Kernel panic - not syncing: panic_on_warn set ... CPU: 1 PID: 4445 Comm: syzkaller195321 Not tainted 4.16.0-rc7+ #5 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 Call Trace: __dump_stack lib/dump_stack.c:17 [inline] dump_stack+0x194/0x24d lib/dump_stack.c:53 panic+0x1e4/0x41c kernel/panic.c:183 __warn+0x1dc/0x200 kernel/panic.c:547 report_bug+0x1f4/0x2b0 lib/bug.c:186 fixup_bug.part.11+0x37/0x80 arch/x86/kernel/traps.c:178 fixup_bug arch/x86/kernel/traps.c:247 [inline] do_error_trap+0x2d7/0x3e0 arch/x86/kernel/traps.c:296 do_invalid_op+0x1b/0x20 arch/x86/kernel/traps.c:315 invalid_op+0x1b/0x40 arch/x86/entry/entry_64.S:986 RIP: 0010:kill_block_super+0xc9/0xf0 fs/super.c:1148 RSP: 0018:8801aecc77a0 EFLAGS: 00010293 RAX: 8801af180040 RBX: 8801aede22c0 RCX: 81b31649 RDX: RSI: 8801b9a76478 RDI: 8801d5527ba0 RBP: 8801aecc77b8 R08: R09: 110035d98ea5 R10: 8801aecc7568 R11: 0001 R12: 8801d5527800 R13: R14: 8801aecc7a10 R15: 8801aede22c0 fuse_kill_sb_blk+0x90/0xb0 fs/fuse/inode.c:1232 deactivate_locked_super+0x88/0xd0 fs/super.c:312 sget_userns+0xbda/0xe40 fs/super.c:522 sget+0xd2/0x120 fs/super.c:557 mount_bdev+0x109/0x370 fs/super.c:1092 fuse_mount_blk+0x34/0x40 fs/fuse/inode.c:1219 mount_fs+0x66/0x2d0 fs/super.c:1222 vfs_kern_mount.part.26+0xc6/0x4a0 fs/namespace.c:1037 vfs_kern_mount fs/namespace.c:2509 [inline] do_new_mount fs/namespace.c:2512 [inline] do_mount+0xea4/0x2bb0 fs/namespace.c:2842 SYSC_mount fs/namespace.c:3058 [inline] SyS_mount+0xab/0x120 fs/namespace.c:3035 do_syscall_64+0x281/0x940 arch/x86/entry/common.c:287 entry_SYSCALL_64_after_hwframe+0x42/0xb7 RIP: 0033:0x442e79 RSP: 002b:7ffed0bc59e8 EFLAGS: 0206 ORIG_RAX: 00a5 RAX: ffda RBX: 7ffed0bc62a0 RCX: 00442e79 RDX: 004a4d6d RSI: 20001740 RDI: 20001780 RBP: R08: 7ffed0bc62a0 R09: R10: R11: 0206 R12: R13: 0008 R14: 1380 R15: 7ffed0bc5b38 Dumping ftrace buffer: (ftrace buffer empty) Kernel Offset: disabled Rebooting in 86400 seconds.. --- This bug is generated by a dumb bot. It may contain errors. See https://goo.gl/tpsmEJ for details. Direct all questions to syzkal...@googlegroups.com. syzbot will keep track of this bug report. If you forgot to add the Reported-by tag, once the fix for this bug is merged into any tree, please reply to this email with: #syz fix: exact-commit-title If you want to test a patch for this bug, please reply with: #syz test: git://repo/address.git branch and provide the patch inline or as an attachment. To mark this as a duplicate of another syzbot report, please reply with: #syz dup: exact-subject-of-another-report If it's a one-off invalid bug report, please reply with: #syz invalid Note: if the crash happens again, it will cause creation of a new bug report. Note: all commands must start from beginning of the line in the email body.
WARNING in kill_block_super
Hello, syzbot hit the following crash on upstream commit 0b412605ef5f5c64b31f19e2910b1d5eba9929c3 (Thu Mar 29 01:07:23 2018 +) Merge tag 'drm-fixes-for-v4.16-rc8' of git://people.freedesktop.org/~airlied/linux syzbot dashboard link: https://syzkaller.appspot.com/bug?extid=5a170e19c963a2e0df79 So far this crash happened 2 times on upstream. C reproducer: https://syzkaller.appspot.com/x/repro.c?id=6168466046320640 syzkaller reproducer: https://syzkaller.appspot.com/x/repro.syz?id=4851040260194304 Raw console output: https://syzkaller.appspot.com/x/log.txt?id=5936700618440704 Kernel config: https://syzkaller.appspot.com/x/.config?id=-8440362230543204781 compiler: gcc (GCC) 7.1.1 20170620 IMPORTANT: if you fix the bug, please add the following tag to the commit: Reported-by: syzbot+5a170e19c963a2e0d...@syzkaller.appspotmail.com It will help syzbot understand when the bug is fixed. See footer for details. If you forward the report, please keep this part and the footer. RAX: ffda RBX: 7ffed0bc62a0 RCX: 00442e79 RDX: 004a4d6d RSI: 20001740 RDI: 20001780 RBP: R08: 7ffed0bc62a0 R09: R10: R11: 0206 R12: R13: 0008 R14: 1380 R15: 7ffed0bc5b38 WARNING: CPU: 1 PID: 4445 at fs/super.c:1148 kill_block_super+0xc9/0xf0 fs/super.c:1148 Kernel panic - not syncing: panic_on_warn set ... CPU: 1 PID: 4445 Comm: syzkaller195321 Not tainted 4.16.0-rc7+ #5 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 Call Trace: __dump_stack lib/dump_stack.c:17 [inline] dump_stack+0x194/0x24d lib/dump_stack.c:53 panic+0x1e4/0x41c kernel/panic.c:183 __warn+0x1dc/0x200 kernel/panic.c:547 report_bug+0x1f4/0x2b0 lib/bug.c:186 fixup_bug.part.11+0x37/0x80 arch/x86/kernel/traps.c:178 fixup_bug arch/x86/kernel/traps.c:247 [inline] do_error_trap+0x2d7/0x3e0 arch/x86/kernel/traps.c:296 do_invalid_op+0x1b/0x20 arch/x86/kernel/traps.c:315 invalid_op+0x1b/0x40 arch/x86/entry/entry_64.S:986 RIP: 0010:kill_block_super+0xc9/0xf0 fs/super.c:1148 RSP: 0018:8801aecc77a0 EFLAGS: 00010293 RAX: 8801af180040 RBX: 8801aede22c0 RCX: 81b31649 RDX: RSI: 8801b9a76478 RDI: 8801d5527ba0 RBP: 8801aecc77b8 R08: R09: 110035d98ea5 R10: 8801aecc7568 R11: 0001 R12: 8801d5527800 R13: R14: 8801aecc7a10 R15: 8801aede22c0 fuse_kill_sb_blk+0x90/0xb0 fs/fuse/inode.c:1232 deactivate_locked_super+0x88/0xd0 fs/super.c:312 sget_userns+0xbda/0xe40 fs/super.c:522 sget+0xd2/0x120 fs/super.c:557 mount_bdev+0x109/0x370 fs/super.c:1092 fuse_mount_blk+0x34/0x40 fs/fuse/inode.c:1219 mount_fs+0x66/0x2d0 fs/super.c:1222 vfs_kern_mount.part.26+0xc6/0x4a0 fs/namespace.c:1037 vfs_kern_mount fs/namespace.c:2509 [inline] do_new_mount fs/namespace.c:2512 [inline] do_mount+0xea4/0x2bb0 fs/namespace.c:2842 SYSC_mount fs/namespace.c:3058 [inline] SyS_mount+0xab/0x120 fs/namespace.c:3035 do_syscall_64+0x281/0x940 arch/x86/entry/common.c:287 entry_SYSCALL_64_after_hwframe+0x42/0xb7 RIP: 0033:0x442e79 RSP: 002b:7ffed0bc59e8 EFLAGS: 0206 ORIG_RAX: 00a5 RAX: ffda RBX: 7ffed0bc62a0 RCX: 00442e79 RDX: 004a4d6d RSI: 20001740 RDI: 20001780 RBP: R08: 7ffed0bc62a0 R09: R10: R11: 0206 R12: R13: 0008 R14: 1380 R15: 7ffed0bc5b38 Dumping ftrace buffer: (ftrace buffer empty) Kernel Offset: disabled Rebooting in 86400 seconds.. --- This bug is generated by a dumb bot. It may contain errors. See https://goo.gl/tpsmEJ for details. Direct all questions to syzkal...@googlegroups.com. syzbot will keep track of this bug report. If you forgot to add the Reported-by tag, once the fix for this bug is merged into any tree, please reply to this email with: #syz fix: exact-commit-title If you want to test a patch for this bug, please reply with: #syz test: git://repo/address.git branch and provide the patch inline or as an attachment. To mark this as a duplicate of another syzbot report, please reply with: #syz dup: exact-subject-of-another-report If it's a one-off invalid bug report, please reply with: #syz invalid Note: if the crash happens again, it will cause creation of a new bug report. Note: all commands must start from beginning of the line in the email body.