* [PATCH] fs: handle shrinker registration failure in sget_userns
@ 2017-11-23 11:52 Michal Hocko
2017-11-23 12:26 ` Jan Kara
` (2 more replies)
0 siblings, 3 replies; 28+ messages in thread
From: Michal Hocko @ 2017-11-23 11:52 UTC (permalink / raw)
To: Al Viro
Cc: Dave Chinner, Jan Kara, Tetsuo Handa, LKML, linux-fsdevel,
Andrew Morton, Michal Hocko
From: Michal Hocko <mhocko@suse.com>
Syzbot has reported NULL ptr dereference during mntput because of
sb shrinker being NULL
CPU: 1 PID: 13231 Comm: syz-executor1 Not tainted 4.14.0-rc8+ #82
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
task: ffff8801d1dbe5c0 task.stack: ffff8801c9e38000
RIP: 0010:__list_del_entry_valid+0x7e/0x150 lib/list_debug.c:51
RSP: 0018:ffff8801c9e3f108 EFLAGS: 00010246
RAX: dffffc0000000000 RBX: 0000000000000000 RCX: 0000000000000000
RDX: 0000000000000000 RSI: ffff8801c53c6f98 RDI: ffff8801c53c6fa0
RBP: ffff8801c9e3f120 R08: 1ffff100393c7d55 R09: 0000000000000004
R10: ffff8801c9e3ef70 R11: 0000000000000000 R12: 0000000000000000
R13: dffffc0000000000 R14: 1ffff100393c7e45 R15: ffff8801c53c6f98
FS: 0000000000000000(0000) GS:ffff8801db300000(0000) knlGS:0000000000000000
CS: 0010 DS: 002b ES: 002b CR0: 0000000080050033
CR2: 00000000dbc23000 CR3: 00000001c7269000 CR4: 00000000001406e0
DR0: 0000000020000000 DR1: 0000000020000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000600
Call Trace:
__list_del_entry include/linux/list.h:117 [inline]
list_del include/linux/list.h:125 [inline]
unregister_shrinker+0x79/0x300 mm/vmscan.c:301
deactivate_locked_super+0x64/0xd0 fs/super.c:308
deactivate_super+0x141/0x1b0 fs/super.c:340
cleanup_mnt+0xb2/0x150 fs/namespace.c:1173
mntput_no_expire+0x6e0/0xa90 fs/namespace.c:1237
mntput fs/namespace.c:1247 [inline]
kern_unmount+0x9c/0xd0 fs/namespace.c:2999
mq_put_mnt+0x37/0x50 ipc/mqueue.c:1609
put_ipc_ns+0x4d/0x150 ipc/namespace.c:163
free_nsproxy+0xc0/0x1f0 kernel/nsproxy.c:180
switch_task_namespaces+0x9d/0xc0 kernel/nsproxy.c:229
exit_task_namespaces+0x17/0x20 kernel/nsproxy.c:234
do_exit+0x9b0/0x1ad0 kernel/exit.c:864
do_group_exit+0x149/0x400 kernel/exit.c:968
Tetsuo has properly pointed out that the real reason is that fault
injection has caused register_shrinker to fail and the error path is not
handled in sget_userns.
Fix the issue by moving the shrinker registration up when the superblock
is allocated and fail early even before we try to register the superblock.
This should be safe wrt. parallel shrinker registration as we are holding
s_umount lock which blocks shrinker invocation.
The issue is very unlikely to trigger in the production because small
allocations do not fail usually.
Debugged-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
Hi,
I am not really familiar with the code but something like this should
work. There are other shrinker users which need a similar treatment
of course. They can be handled separately. The failure path is rather
unlikely to lose sleep over.
fs/super.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/fs/super.c b/fs/super.c
index d4e33e8f1e6f..cfeb0e12b193 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -489,6 +489,7 @@ struct super_block *sget_userns(struct file_system_type *type,
continue;
if (user_ns != old->s_user_ns) {
spin_unlock(&sb_lock);
+ unregister_shrinker(&s->s_shrink);
destroy_unused_super(s);
return ERR_PTR(-EBUSY);
}
@@ -503,12 +504,18 @@ struct super_block *sget_userns(struct file_system_type *type,
s = alloc_super(type, (flags & ~SB_SUBMOUNT), user_ns);
if (!s)
return ERR_PTR(-ENOMEM);
+ if (register_shrinker(&s->s_shrink)) {
+ spin_unlock(&sb_lock);
+ destroy_unused_super(s);
+ return ERR_PTR(-ENOMEM);
+ }
goto retry;
}
err = set(s, data);
if (err) {
spin_unlock(&sb_lock);
+ unregister_shrinker(&s->s_shrink);
destroy_unused_super(s);
return ERR_PTR(err);
}
@@ -518,7 +525,6 @@ struct super_block *sget_userns(struct file_system_type *type,
hlist_add_head(&s->s_instances, &type->fs_supers);
spin_unlock(&sb_lock);
get_filesystem(type);
- register_shrinker(&s->s_shrink);
return s;
}
--
2.15.0
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH] fs: handle shrinker registration failure in sget_userns
2017-11-23 11:52 [PATCH] fs: handle shrinker registration failure in sget_userns Michal Hocko
@ 2017-11-23 12:26 ` Jan Kara
2017-11-23 12:45 ` Michal Hocko
2017-11-23 14:35 ` [PATCH v2] " Michal Hocko
2017-11-23 14:47 ` [PATCH] " Al Viro
2 siblings, 1 reply; 28+ messages in thread
From: Jan Kara @ 2017-11-23 12:26 UTC (permalink / raw)
To: Michal Hocko
Cc: Al Viro, Dave Chinner, Jan Kara, Tetsuo Handa, LKML,
linux-fsdevel, Andrew Morton, Michal Hocko
On Thu 23-11-17 12:52:47, Michal Hocko wrote:
> From: Michal Hocko <mhocko@suse.com>
>
> Syzbot has reported NULL ptr dereference during mntput because of
> sb shrinker being NULL
> CPU: 1 PID: 13231 Comm: syz-executor1 Not tainted 4.14.0-rc8+ #82
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> task: ffff8801d1dbe5c0 task.stack: ffff8801c9e38000
> RIP: 0010:__list_del_entry_valid+0x7e/0x150 lib/list_debug.c:51
> RSP: 0018:ffff8801c9e3f108 EFLAGS: 00010246
> RAX: dffffc0000000000 RBX: 0000000000000000 RCX: 0000000000000000
> RDX: 0000000000000000 RSI: ffff8801c53c6f98 RDI: ffff8801c53c6fa0
> RBP: ffff8801c9e3f120 R08: 1ffff100393c7d55 R09: 0000000000000004
> R10: ffff8801c9e3ef70 R11: 0000000000000000 R12: 0000000000000000
> R13: dffffc0000000000 R14: 1ffff100393c7e45 R15: ffff8801c53c6f98
> FS: 0000000000000000(0000) GS:ffff8801db300000(0000) knlGS:0000000000000000
> CS: 0010 DS: 002b ES: 002b CR0: 0000000080050033
> CR2: 00000000dbc23000 CR3: 00000001c7269000 CR4: 00000000001406e0
> DR0: 0000000020000000 DR1: 0000000020000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000600
> Call Trace:
> __list_del_entry include/linux/list.h:117 [inline]
> list_del include/linux/list.h:125 [inline]
> unregister_shrinker+0x79/0x300 mm/vmscan.c:301
> deactivate_locked_super+0x64/0xd0 fs/super.c:308
> deactivate_super+0x141/0x1b0 fs/super.c:340
> cleanup_mnt+0xb2/0x150 fs/namespace.c:1173
> mntput_no_expire+0x6e0/0xa90 fs/namespace.c:1237
> mntput fs/namespace.c:1247 [inline]
> kern_unmount+0x9c/0xd0 fs/namespace.c:2999
> mq_put_mnt+0x37/0x50 ipc/mqueue.c:1609
> put_ipc_ns+0x4d/0x150 ipc/namespace.c:163
> free_nsproxy+0xc0/0x1f0 kernel/nsproxy.c:180
> switch_task_namespaces+0x9d/0xc0 kernel/nsproxy.c:229
> exit_task_namespaces+0x17/0x20 kernel/nsproxy.c:234
> do_exit+0x9b0/0x1ad0 kernel/exit.c:864
> do_group_exit+0x149/0x400 kernel/exit.c:968
>
> Tetsuo has properly pointed out that the real reason is that fault
> injection has caused register_shrinker to fail and the error path is not
> handled in sget_userns.
>
> Fix the issue by moving the shrinker registration up when the superblock
> is allocated and fail early even before we try to register the superblock.
> This should be safe wrt. parallel shrinker registration as we are holding
> s_umount lock which blocks shrinker invocation.
>
> The issue is very unlikely to trigger in the production because small
> allocations do not fail usually.
>
> Debugged-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
>
> Hi,
> I am not really familiar with the code but something like this should
> work. There are other shrinker users which need a similar treatment
> of course. They can be handled separately. The failure path is rather
> unlikely to lose sleep over.
>
> fs/super.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/fs/super.c b/fs/super.c
> index d4e33e8f1e6f..cfeb0e12b193 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -489,6 +489,7 @@ struct super_block *sget_userns(struct file_system_type *type,
> continue;
> if (user_ns != old->s_user_ns) {
> spin_unlock(&sb_lock);
> + unregister_shrinker(&s->s_shrink);
This is wrong as 's' can be NULL at this point. I think the right fix is to
move unregister_shrinker() into destroy_unused_super(). But for that we
need a reliable way to detect whether the shrinker has been already
registered - possibly by initializing sb->shrinker.list in alloc_super()
and then checking for list_empty() in destroy_unused_super().
Also I'd note that early shrinker registration breaks assumption of
destroy_unused_super() that nobody could have seen the superblock -
shrinkers could have - but since shrinker code doesn't use RCU to access
the superblock, we are fine. But still comment before
destroy_unused_super() should be probably updated.
Honza
> destroy_unused_super(s);
> return ERR_PTR(-EBUSY);
> }
> @@ -503,12 +504,18 @@ struct super_block *sget_userns(struct file_system_type *type,
> s = alloc_super(type, (flags & ~SB_SUBMOUNT), user_ns);
> if (!s)
> return ERR_PTR(-ENOMEM);
> + if (register_shrinker(&s->s_shrink)) {
> + spin_unlock(&sb_lock);
> + destroy_unused_super(s);
> + return ERR_PTR(-ENOMEM);
> + }
> goto retry;
> }
>
> err = set(s, data);
> if (err) {
> spin_unlock(&sb_lock);
> + unregister_shrinker(&s->s_shrink);
> destroy_unused_super(s);
> return ERR_PTR(err);
> }
> @@ -518,7 +525,6 @@ struct super_block *sget_userns(struct file_system_type *type,
> hlist_add_head(&s->s_instances, &type->fs_supers);
> spin_unlock(&sb_lock);
> get_filesystem(type);
> - register_shrinker(&s->s_shrink);
> return s;
> }
>
> --
> 2.15.0
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH] fs: handle shrinker registration failure in sget_userns
2017-11-23 12:26 ` Jan Kara
@ 2017-11-23 12:45 ` Michal Hocko
2017-11-23 12:53 ` Michal Hocko
0 siblings, 1 reply; 28+ messages in thread
From: Michal Hocko @ 2017-11-23 12:45 UTC (permalink / raw)
To: Jan Kara
Cc: Al Viro, Dave Chinner, Tetsuo Handa, LKML, linux-fsdevel, Andrew Morton
On Thu 23-11-17 13:26:16, Jan Kara wrote:
> On Thu 23-11-17 12:52:47, Michal Hocko wrote:
[...]
> > @@ -489,6 +489,7 @@ struct super_block *sget_userns(struct file_system_type *type,
> > continue;
> > if (user_ns != old->s_user_ns) {
> > spin_unlock(&sb_lock);
> > + unregister_shrinker(&s->s_shrink);
>
> This is wrong as 's' can be NULL at this point.
Ohh, I've seen destroy_unused_super(s) and thought it operates on
non-NULL s. My bad.
> I think the right fix is to
> move unregister_shrinker() into destroy_unused_super(). But for that we
> need a reliable way to detect whether the shrinker has been already
> registered - possibly by initializing sb->shrinker.list in alloc_super()
> and then checking for list_empty() in destroy_unused_super().
Yeah, that makes sense.
> Also I'd note that early shrinker registration breaks assumption of
> destroy_unused_super() that nobody could have seen the superblock -
> shrinkers could have - but since shrinker code doesn't use RCU to access
> the superblock, we are fine. But still comment before
> destroy_unused_super() should be probably updated.
Right. Thanks a lot for the review Jan!
What about the following?
---
>From cffea62e7f8605c370c8115afae530a1831e75f3 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Thu, 23 Nov 2017 12:28:35 +0100
Subject: [PATCH] fs: handle shrinker registration failure in sget_userns
Syzbot has reported NULL ptr dereference during mntput because of
sb shrinker being NULL
CPU: 1 PID: 13231 Comm: syz-executor1 Not tainted 4.14.0-rc8+ #82
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
task: ffff8801d1dbe5c0 task.stack: ffff8801c9e38000
RIP: 0010:__list_del_entry_valid+0x7e/0x150 lib/list_debug.c:51
RSP: 0018:ffff8801c9e3f108 EFLAGS: 00010246
RAX: dffffc0000000000 RBX: 0000000000000000 RCX: 0000000000000000
RDX: 0000000000000000 RSI: ffff8801c53c6f98 RDI: ffff8801c53c6fa0
RBP: ffff8801c9e3f120 R08: 1ffff100393c7d55 R09: 0000000000000004
R10: ffff8801c9e3ef70 R11: 0000000000000000 R12: 0000000000000000
R13: dffffc0000000000 R14: 1ffff100393c7e45 R15: ffff8801c53c6f98
FS: 0000000000000000(0000) GS:ffff8801db300000(0000) knlGS:0000000000000000
CS: 0010 DS: 002b ES: 002b CR0: 0000000080050033
CR2: 00000000dbc23000 CR3: 00000001c7269000 CR4: 00000000001406e0
DR0: 0000000020000000 DR1: 0000000020000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000600
Call Trace:
__list_del_entry include/linux/list.h:117 [inline]
list_del include/linux/list.h:125 [inline]
unregister_shrinker+0x79/0x300 mm/vmscan.c:301
deactivate_locked_super+0x64/0xd0 fs/super.c:308
deactivate_super+0x141/0x1b0 fs/super.c:340
cleanup_mnt+0xb2/0x150 fs/namespace.c:1173
mntput_no_expire+0x6e0/0xa90 fs/namespace.c:1237
mntput fs/namespace.c:1247 [inline]
kern_unmount+0x9c/0xd0 fs/namespace.c:2999
mq_put_mnt+0x37/0x50 ipc/mqueue.c:1609
put_ipc_ns+0x4d/0x150 ipc/namespace.c:163
free_nsproxy+0xc0/0x1f0 kernel/nsproxy.c:180
switch_task_namespaces+0x9d/0xc0 kernel/nsproxy.c:229
exit_task_namespaces+0x17/0x20 kernel/nsproxy.c:234
do_exit+0x9b0/0x1ad0 kernel/exit.c:864
do_group_exit+0x149/0x400 kernel/exit.c:968
Tetsuo has properly pointed out that the real reason is that fault
injection has caused register_shrinker to fail and the error path is not
handled in sget_userns.
Fix the issue by moving the shrinker registration up when the superblock
is allocated and fail early even before we try to register the superblock.
This should be safe wrt. parallel shrinker invocation as we are holding
s_umount lock which blocks shrinker invocation.
The issue is very unlikely to trigger in the production because small
allocations do not fail usually.
Debugged-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
fs/super.c | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/fs/super.c b/fs/super.c
index d4e33e8f1e6f..a306b5fef1ea 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -155,11 +155,19 @@ static void destroy_super_rcu(struct rcu_head *head)
schedule_work(&s->destroy_work);
}
-/* Free a superblock that has never been seen by anyone */
+/*
+ * Free a superblock that has never been seen by anyone. Note that shrinkers
+ * could have been invoked already but we rely on s_umount to not actually
+ * touch it.
+ */
static void destroy_unused_super(struct super_block *s)
{
if (!s)
return;
+
+ if (!list_empty(&s->s_shrink.list))
+ unregister_shrinker(&s->s_shrink);
+
up_write(&s->s_umount);
list_lru_destroy(&s->s_dentry_lru);
list_lru_destroy(&s->s_inode_lru);
@@ -252,6 +260,7 @@ 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;
+ INIT_LIST_HEAD(&s->s_shrink.list);
return s;
fail:
@@ -503,6 +512,11 @@ struct super_block *sget_userns(struct file_system_type *type,
s = alloc_super(type, (flags & ~SB_SUBMOUNT), user_ns);
if (!s)
return ERR_PTR(-ENOMEM);
+ if (register_shrinker(&s->s_shrink)) {
+ spin_unlock(&sb_lock);
+ destroy_unused_super(s);
+ return ERR_PTR(-ENOMEM);
+ }
goto retry;
}
@@ -518,7 +532,6 @@ struct super_block *sget_userns(struct file_system_type *type,
hlist_add_head(&s->s_instances, &type->fs_supers);
spin_unlock(&sb_lock);
get_filesystem(type);
- register_shrinker(&s->s_shrink);
return s;
}
--
2.15.0
--
Michal Hocko
SUSE Labs
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH] fs: handle shrinker registration failure in sget_userns
2017-11-23 12:45 ` Michal Hocko
@ 2017-11-23 12:53 ` Michal Hocko
2017-11-23 12:56 ` Jan Kara
0 siblings, 1 reply; 28+ messages in thread
From: Michal Hocko @ 2017-11-23 12:53 UTC (permalink / raw)
To: Jan Kara
Cc: Al Viro, Dave Chinner, Tetsuo Handa, LKML, linux-fsdevel, Andrew Morton
On Thu 23-11-17 13:45:41, Michal Hocko wrote:
[...]
> What about the following?
Dohh, a rebase artifact sneaked in. So let me try again. Sorry about
spamming :/
---
>From 467be16ca5165613daf292a68592e3b5bc7252c5 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Thu, 23 Nov 2017 12:28:35 +0100
Subject: [PATCH] fs: handle shrinker registration failure in sget_userns
Syzbot has reported NULL ptr dereference during mntput because of
sb shrinker being NULL
CPU: 1 PID: 13231 Comm: syz-executor1 Not tainted 4.14.0-rc8+ #82
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
task: ffff8801d1dbe5c0 task.stack: ffff8801c9e38000
RIP: 0010:__list_del_entry_valid+0x7e/0x150 lib/list_debug.c:51
RSP: 0018:ffff8801c9e3f108 EFLAGS: 00010246
RAX: dffffc0000000000 RBX: 0000000000000000 RCX: 0000000000000000
RDX: 0000000000000000 RSI: ffff8801c53c6f98 RDI: ffff8801c53c6fa0
RBP: ffff8801c9e3f120 R08: 1ffff100393c7d55 R09: 0000000000000004
R10: ffff8801c9e3ef70 R11: 0000000000000000 R12: 0000000000000000
R13: dffffc0000000000 R14: 1ffff100393c7e45 R15: ffff8801c53c6f98
FS: 0000000000000000(0000) GS:ffff8801db300000(0000) knlGS:0000000000000000
CS: 0010 DS: 002b ES: 002b CR0: 0000000080050033
CR2: 00000000dbc23000 CR3: 00000001c7269000 CR4: 00000000001406e0
DR0: 0000000020000000 DR1: 0000000020000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000600
Call Trace:
__list_del_entry include/linux/list.h:117 [inline]
list_del include/linux/list.h:125 [inline]
unregister_shrinker+0x79/0x300 mm/vmscan.c:301
deactivate_locked_super+0x64/0xd0 fs/super.c:308
deactivate_super+0x141/0x1b0 fs/super.c:340
cleanup_mnt+0xb2/0x150 fs/namespace.c:1173
mntput_no_expire+0x6e0/0xa90 fs/namespace.c:1237
mntput fs/namespace.c:1247 [inline]
kern_unmount+0x9c/0xd0 fs/namespace.c:2999
mq_put_mnt+0x37/0x50 ipc/mqueue.c:1609
put_ipc_ns+0x4d/0x150 ipc/namespace.c:163
free_nsproxy+0xc0/0x1f0 kernel/nsproxy.c:180
switch_task_namespaces+0x9d/0xc0 kernel/nsproxy.c:229
exit_task_namespaces+0x17/0x20 kernel/nsproxy.c:234
do_exit+0x9b0/0x1ad0 kernel/exit.c:864
do_group_exit+0x149/0x400 kernel/exit.c:968
Tetsuo has properly pointed out that the real reason is that fault
injection has caused register_shrinker to fail and the error path is not
handled in sget_userns.
Fix the issue by moving the shrinker registration up when the superblock
is allocated and fail early even before we try to register the superblock.
This should be safe wrt. parallel shrinker invocation as we are holding
s_umount lock which blocks shrinker invocation.
The issue is very unlikely to trigger in the production because small
allocations do not fail usually.
Debugged-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
fs/super.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/fs/super.c b/fs/super.c
index d4e33e8f1e6f..80b118cc4bb6 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -155,11 +155,19 @@ static void destroy_super_rcu(struct rcu_head *head)
schedule_work(&s->destroy_work);
}
-/* Free a superblock that has never been seen by anyone */
+/*
+ * Free a superblock that has never been seen by anyone. Note that shrinkers
+ * could have been invoked already but we rely on s_umount to not actually
+ * touch it.
+ */
static void destroy_unused_super(struct super_block *s)
{
if (!s)
return;
+
+ if (!list_empty(&s->s_shrink.list))
+ unregister_shrinker(&s->s_shrink);
+
up_write(&s->s_umount);
list_lru_destroy(&s->s_dentry_lru);
list_lru_destroy(&s->s_inode_lru);
@@ -252,6 +260,7 @@ 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;
+ INIT_LIST_HEAD(&s->s_shrink.list);
return s;
fail:
@@ -503,6 +512,10 @@ struct super_block *sget_userns(struct file_system_type *type,
s = alloc_super(type, (flags & ~SB_SUBMOUNT), user_ns);
if (!s)
return ERR_PTR(-ENOMEM);
+ if (register_shrinker(&s->s_shrink)) {
+ destroy_unused_super(s);
+ return ERR_PTR(-ENOMEM);
+ }
goto retry;
}
@@ -518,7 +531,6 @@ struct super_block *sget_userns(struct file_system_type *type,
hlist_add_head(&s->s_instances, &type->fs_supers);
spin_unlock(&sb_lock);
get_filesystem(type);
- register_shrinker(&s->s_shrink);
return s;
}
--
2.15.0
--
Michal Hocko
SUSE Labs
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH] fs: handle shrinker registration failure in sget_userns
2017-11-23 12:53 ` Michal Hocko
@ 2017-11-23 12:56 ` Jan Kara
2017-11-23 13:35 ` Tetsuo Handa
0 siblings, 1 reply; 28+ messages in thread
From: Jan Kara @ 2017-11-23 12:56 UTC (permalink / raw)
To: Michal Hocko
Cc: Jan Kara, Al Viro, Dave Chinner, Tetsuo Handa, LKML,
linux-fsdevel, Andrew Morton
On Thu 23-11-17 13:53:45, Michal Hocko wrote:
> On Thu 23-11-17 13:45:41, Michal Hocko wrote:
> [...]
> > What about the following?
>
> Dohh, a rebase artifact sneaked in. So let me try again. Sorry about
> spamming :/
> ---
> From 467be16ca5165613daf292a68592e3b5bc7252c5 Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.com>
> Date: Thu, 23 Nov 2017 12:28:35 +0100
> Subject: [PATCH] fs: handle shrinker registration failure in sget_userns
>
> Syzbot has reported NULL ptr dereference during mntput because of
> sb shrinker being NULL
> CPU: 1 PID: 13231 Comm: syz-executor1 Not tainted 4.14.0-rc8+ #82
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> task: ffff8801d1dbe5c0 task.stack: ffff8801c9e38000
> RIP: 0010:__list_del_entry_valid+0x7e/0x150 lib/list_debug.c:51
> RSP: 0018:ffff8801c9e3f108 EFLAGS: 00010246
> RAX: dffffc0000000000 RBX: 0000000000000000 RCX: 0000000000000000
> RDX: 0000000000000000 RSI: ffff8801c53c6f98 RDI: ffff8801c53c6fa0
> RBP: ffff8801c9e3f120 R08: 1ffff100393c7d55 R09: 0000000000000004
> R10: ffff8801c9e3ef70 R11: 0000000000000000 R12: 0000000000000000
> R13: dffffc0000000000 R14: 1ffff100393c7e45 R15: ffff8801c53c6f98
> FS: 0000000000000000(0000) GS:ffff8801db300000(0000) knlGS:0000000000000000
> CS: 0010 DS: 002b ES: 002b CR0: 0000000080050033
> CR2: 00000000dbc23000 CR3: 00000001c7269000 CR4: 00000000001406e0
> DR0: 0000000020000000 DR1: 0000000020000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000600
> Call Trace:
> __list_del_entry include/linux/list.h:117 [inline]
> list_del include/linux/list.h:125 [inline]
> unregister_shrinker+0x79/0x300 mm/vmscan.c:301
> deactivate_locked_super+0x64/0xd0 fs/super.c:308
> deactivate_super+0x141/0x1b0 fs/super.c:340
> cleanup_mnt+0xb2/0x150 fs/namespace.c:1173
> mntput_no_expire+0x6e0/0xa90 fs/namespace.c:1237
> mntput fs/namespace.c:1247 [inline]
> kern_unmount+0x9c/0xd0 fs/namespace.c:2999
> mq_put_mnt+0x37/0x50 ipc/mqueue.c:1609
> put_ipc_ns+0x4d/0x150 ipc/namespace.c:163
> free_nsproxy+0xc0/0x1f0 kernel/nsproxy.c:180
> switch_task_namespaces+0x9d/0xc0 kernel/nsproxy.c:229
> exit_task_namespaces+0x17/0x20 kernel/nsproxy.c:234
> do_exit+0x9b0/0x1ad0 kernel/exit.c:864
> do_group_exit+0x149/0x400 kernel/exit.c:968
>
> Tetsuo has properly pointed out that the real reason is that fault
> injection has caused register_shrinker to fail and the error path is not
> handled in sget_userns.
>
> Fix the issue by moving the shrinker registration up when the superblock
> is allocated and fail early even before we try to register the superblock.
> This should be safe wrt. parallel shrinker invocation as we are holding
> s_umount lock which blocks shrinker invocation.
>
> The issue is very unlikely to trigger in the production because small
> allocations do not fail usually.
>
> Debugged-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Signed-off-by: Michal Hocko <mhocko@suse.com>
Looks good to me now. You can add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
> ---
> fs/super.c | 16 ++++++++++++++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/fs/super.c b/fs/super.c
> index d4e33e8f1e6f..80b118cc4bb6 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -155,11 +155,19 @@ static void destroy_super_rcu(struct rcu_head *head)
> schedule_work(&s->destroy_work);
> }
>
> -/* Free a superblock that has never been seen by anyone */
> +/*
> + * Free a superblock that has never been seen by anyone. Note that shrinkers
> + * could have been invoked already but we rely on s_umount to not actually
> + * touch it.
> + */
> static void destroy_unused_super(struct super_block *s)
> {
> if (!s)
> return;
> +
> + if (!list_empty(&s->s_shrink.list))
> + unregister_shrinker(&s->s_shrink);
> +
> up_write(&s->s_umount);
> list_lru_destroy(&s->s_dentry_lru);
> list_lru_destroy(&s->s_inode_lru);
> @@ -252,6 +260,7 @@ 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;
> + INIT_LIST_HEAD(&s->s_shrink.list);
> return s;
>
> fail:
> @@ -503,6 +512,10 @@ struct super_block *sget_userns(struct file_system_type *type,
> s = alloc_super(type, (flags & ~SB_SUBMOUNT), user_ns);
> if (!s)
> return ERR_PTR(-ENOMEM);
> + if (register_shrinker(&s->s_shrink)) {
> + destroy_unused_super(s);
> + return ERR_PTR(-ENOMEM);
> + }
> goto retry;
> }
>
> @@ -518,7 +531,6 @@ struct super_block *sget_userns(struct file_system_type *type,
> hlist_add_head(&s->s_instances, &type->fs_supers);
> spin_unlock(&sb_lock);
> get_filesystem(type);
> - register_shrinker(&s->s_shrink);
> return s;
> }
>
> --
> 2.15.0
>
> --
> Michal Hocko
> SUSE Labs
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH] fs: handle shrinker registration failure in sget_userns
2017-11-23 12:56 ` Jan Kara
@ 2017-11-23 13:35 ` Tetsuo Handa
2017-11-23 13:46 ` Michal Hocko
0 siblings, 1 reply; 28+ messages in thread
From: Tetsuo Handa @ 2017-11-23 13:35 UTC (permalink / raw)
To: jack, mhocko; +Cc: viro, david, linux-kernel, linux-fsdevel, akpm
Jan Kara wrote:
> Looks good to me now. You can add:
>
> Reviewed-by: Jan Kara <jack@suse.cz>
>
It does not look good to me, for "goto fail" can call
destroy_unused_super() before s->s_shrink.list is initialized.
Also, the comment block saying "this object isn't exposed yet"
wants to be updated?
---
fs/super.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/fs/super.c b/fs/super.c
index 80b118c..44f0c6b 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -197,6 +197,7 @@ static struct super_block *alloc_super(struct file_system_type *type, int flags,
if (!s)
return NULL;
+ INIT_LIST_HEAD(&s->s_shrink.list);
INIT_LIST_HEAD(&s->s_mounts);
s->s_user_ns = get_user_ns(user_ns);
@@ -260,9 +261,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;
- INIT_LIST_HEAD(&s->s_shrink.list);
- return s;
-
+ if (register_shrinker(&s->s_shrink) == 0)
+ return s;
fail:
destroy_unused_super(s);
return NULL;
@@ -512,10 +512,6 @@ struct super_block *sget_userns(struct file_system_type *type,
s = alloc_super(type, (flags & ~SB_SUBMOUNT), user_ns);
if (!s)
return ERR_PTR(-ENOMEM);
- if (register_shrinker(&s->s_shrink)) {
- destroy_unused_super(s);
- return ERR_PTR(-ENOMEM);
- }
goto retry;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH] fs: handle shrinker registration failure in sget_userns
2017-11-23 13:35 ` Tetsuo Handa
@ 2017-11-23 13:46 ` Michal Hocko
2017-11-23 13:57 ` Tetsuo Handa
0 siblings, 1 reply; 28+ messages in thread
From: Michal Hocko @ 2017-11-23 13:46 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: jack, viro, david, linux-kernel, linux-fsdevel, akpm
On Thu 23-11-17 22:35:34, Tetsuo Handa wrote:
> Jan Kara wrote:
> > Looks good to me now. You can add:
> >
> > Reviewed-by: Jan Kara <jack@suse.cz>
> >
>
> It does not look good to me, for "goto fail" can call
> destroy_unused_super() before s->s_shrink.list is initialized.
> Also, the comment block saying "this object isn't exposed yet"
> wants to be updated?
>
> ---
> fs/super.c | 10 +++-------
> 1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/fs/super.c b/fs/super.c
> index 80b118c..44f0c6b 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -197,6 +197,7 @@ static struct super_block *alloc_super(struct file_system_type *type, int flags,
> if (!s)
> return NULL;
>
> + INIT_LIST_HEAD(&s->s_shrink.list);
> INIT_LIST_HEAD(&s->s_mounts);
> s->s_user_ns = get_user_ns(user_ns);
>
You are right. I will move it.
> @@ -260,9 +261,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;
> - INIT_LIST_HEAD(&s->s_shrink.list);
> - return s;
> -
> + if (register_shrinker(&s->s_shrink) == 0)
> + return s;
> fail:
> destroy_unused_super(s);
> return NULL;
But I am not sure this is correct. So what protects shrinker invocation
while the object is not initialized yet?
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] fs: handle shrinker registration failure in sget_userns
2017-11-23 13:46 ` Michal Hocko
@ 2017-11-23 13:57 ` Tetsuo Handa
2017-11-23 14:06 ` Michal Hocko
0 siblings, 1 reply; 28+ messages in thread
From: Tetsuo Handa @ 2017-11-23 13:57 UTC (permalink / raw)
To: mhocko; +Cc: jack, viro, david, linux-kernel, linux-fsdevel, akpm
Michal Hocko wrote:
> > @@ -260,9 +261,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;
> > - INIT_LIST_HEAD(&s->s_shrink.list);
> > - return s;
> > -
> > + if (register_shrinker(&s->s_shrink) == 0)
> > + return s;
> > fail:
> > destroy_unused_super(s);
> > return NULL;
>
> But I am not sure this is correct. So what protects shrinker invocation
> while the object is not initialized yet?
Then, what protects shrinker invocation in your patch?
Your patch is calling register_shrinker() immediately after
alloc_super() succeeds. My patch just moved register_shrinker()
into alloc_super().
@@ -503,6 +512,10 @@ struct super_block *sget_userns(struct file_system_type *type,
s = alloc_super(type, (flags & ~SB_SUBMOUNT), user_ns);
if (!s)
return ERR_PTR(-ENOMEM);
+ if (register_shrinker(&s->s_shrink)) {
+ destroy_unused_super(s);
+ return ERR_PTR(-ENOMEM);
+ }
goto retry;
}
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH] fs: handle shrinker registration failure in sget_userns
2017-11-23 13:57 ` Tetsuo Handa
@ 2017-11-23 14:06 ` Michal Hocko
2017-11-23 14:10 ` Tetsuo Handa
0 siblings, 1 reply; 28+ messages in thread
From: Michal Hocko @ 2017-11-23 14:06 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: jack, viro, david, linux-kernel, linux-fsdevel, akpm
On Thu 23-11-17 22:57:06, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > > @@ -260,9 +261,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;
> > > - INIT_LIST_HEAD(&s->s_shrink.list);
> > > - return s;
> > > -
> > > + if (register_shrinker(&s->s_shrink) == 0)
> > > + return s;
> > > fail:
> > > destroy_unused_super(s);
> > > return NULL;
> >
> > But I am not sure this is correct. So what protects shrinker invocation
> > while the object is not initialized yet?
>
> Then, what protects shrinker invocation in your patch?
It is s_umount lock but that one is alreay held at the point where you
suggested register_shrinker. My bad, I could have noticed that. Feel
free to take over and send a patch. Considering I've screwed several
times already I do not feel I am the right one to send the fix.
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] fs: handle shrinker registration failure in sget_userns
2017-11-23 14:06 ` Michal Hocko
@ 2017-11-23 14:10 ` Tetsuo Handa
0 siblings, 0 replies; 28+ messages in thread
From: Tetsuo Handa @ 2017-11-23 14:10 UTC (permalink / raw)
To: mhocko; +Cc: jack, viro, david, linux-kernel, linux-fsdevel, akpm
Michal Hocko wrote:
> On Thu 23-11-17 22:57:06, Tetsuo Handa wrote:
> > Michal Hocko wrote:
> > > > @@ -260,9 +261,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;
> > > > - INIT_LIST_HEAD(&s->s_shrink.list);
> > > > - return s;
> > > > -
> > > > + if (register_shrinker(&s->s_shrink) == 0)
> > > > + return s;
> > > > fail:
> > > > destroy_unused_super(s);
> > > > return NULL;
> > >
> > > But I am not sure this is correct. So what protects shrinker invocation
> > > while the object is not initialized yet?
> >
> > Then, what protects shrinker invocation in your patch?
>
> It is s_umount lock but that one is alreay held at the point where you
> suggested register_shrinker. My bad, I could have noticed that. Feel
> free to take over and send a patch. Considering I've screwed several
> times already I do not feel I am the right one to send the fix.
>
I will wait for your posting. I feel we want to update the comment block
saying "this object isn't exposed yet", for it is confusing that we
already exposed the shrinker inside the object.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] fs: handle shrinker registration failure in sget_userns
2017-11-23 11:52 [PATCH] fs: handle shrinker registration failure in sget_userns Michal Hocko
2017-11-23 12:26 ` Jan Kara
@ 2017-11-23 14:35 ` Michal Hocko
2017-11-23 14:55 ` Al Viro
2017-11-23 14:47 ` [PATCH] " Al Viro
2 siblings, 1 reply; 28+ messages in thread
From: Michal Hocko @ 2017-11-23 14:35 UTC (permalink / raw)
To: Al Viro
Cc: Dave Chinner, Jan Kara, Tetsuo Handa, LKML, linux-fsdevel, Andrew Morton
Hopefully less screwed version. But as I've said I am not really
familiar with the code and do not feel competent to change it so please
be very careful here. I've moved the shrinker registration to
alloc_super which turned out to be simpler.
---
>From f2a86a9dc45853149d4f29a5ecff77ec4c827b9f Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Thu, 23 Nov 2017 12:28:35 +0100
Subject: [PATCH] fs: handle shrinker registration failure in sget_userns
Syzbot has reported NULL ptr dereference during mntput because of
sb shrinker being NULL
CPU: 1 PID: 13231 Comm: syz-executor1 Not tainted 4.14.0-rc8+ #82
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
task: ffff8801d1dbe5c0 task.stack: ffff8801c9e38000
RIP: 0010:__list_del_entry_valid+0x7e/0x150 lib/list_debug.c:51
RSP: 0018:ffff8801c9e3f108 EFLAGS: 00010246
RAX: dffffc0000000000 RBX: 0000000000000000 RCX: 0000000000000000
RDX: 0000000000000000 RSI: ffff8801c53c6f98 RDI: ffff8801c53c6fa0
RBP: ffff8801c9e3f120 R08: 1ffff100393c7d55 R09: 0000000000000004
R10: ffff8801c9e3ef70 R11: 0000000000000000 R12: 0000000000000000
R13: dffffc0000000000 R14: 1ffff100393c7e45 R15: ffff8801c53c6f98
FS: 0000000000000000(0000) GS:ffff8801db300000(0000) knlGS:0000000000000000
CS: 0010 DS: 002b ES: 002b CR0: 0000000080050033
CR2: 00000000dbc23000 CR3: 00000001c7269000 CR4: 00000000001406e0
DR0: 0000000020000000 DR1: 0000000020000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000600
Call Trace:
__list_del_entry include/linux/list.h:117 [inline]
list_del include/linux/list.h:125 [inline]
unregister_shrinker+0x79/0x300 mm/vmscan.c:301
deactivate_locked_super+0x64/0xd0 fs/super.c:308
deactivate_super+0x141/0x1b0 fs/super.c:340
cleanup_mnt+0xb2/0x150 fs/namespace.c:1173
mntput_no_expire+0x6e0/0xa90 fs/namespace.c:1237
mntput fs/namespace.c:1247 [inline]
kern_unmount+0x9c/0xd0 fs/namespace.c:2999
mq_put_mnt+0x37/0x50 ipc/mqueue.c:1609
put_ipc_ns+0x4d/0x150 ipc/namespace.c:163
free_nsproxy+0xc0/0x1f0 kernel/nsproxy.c:180
switch_task_namespaces+0x9d/0xc0 kernel/nsproxy.c:229
exit_task_namespaces+0x17/0x20 kernel/nsproxy.c:234
do_exit+0x9b0/0x1ad0 kernel/exit.c:864
do_group_exit+0x149/0x400 kernel/exit.c:968
Tetsuo has properly pointed out that the real reason is that fault
injection has caused register_shrinker to fail and the error path is not
handled in sget_userns.
Fix the issue by moving the shrinker registration up when the superblock
is allocated and fail early even before we try to register the superblock.
This should be safe wrt. parallel shrinker invocation as we are holding
s_umount lock which blocks shrinker invocation.
The issue is very unlikely to trigger in the production because small
allocations do not fail usually.
Debugged-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
fs/super.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/fs/super.c b/fs/super.c
index d4e33e8f1e6f..29edf3d1875b 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -155,11 +155,19 @@ static void destroy_super_rcu(struct rcu_head *head)
schedule_work(&s->destroy_work);
}
-/* Free a superblock that has never been seen by anyone */
+/*
+ * Free a superblock that has never been seen by anyone. Note that shrinkers
+ * could have been invoked already but we rely on s_umount to not actually
+ * touch it.
+ */
static void destroy_unused_super(struct super_block *s)
{
if (!s)
return;
+
+ if (!list_empty(&s->s_shrink.list))
+ unregister_shrinker(&s->s_shrink);
+
up_write(&s->s_umount);
list_lru_destroy(&s->s_dentry_lru);
list_lru_destroy(&s->s_inode_lru);
@@ -190,6 +198,7 @@ static struct super_block *alloc_super(struct file_system_type *type, int flags,
return NULL;
INIT_LIST_HEAD(&s->s_mounts);
+ INIT_LIST_HEAD(&s->s_shrink.list);
s->s_user_ns = get_user_ns(user_ns);
if (security_sb_alloc(s))
@@ -252,6 +261,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 (register_shrinker(&s->s_shrink))
+ goto fail;
return s;
fail:
@@ -518,7 +529,6 @@ struct super_block *sget_userns(struct file_system_type *type,
hlist_add_head(&s->s_instances, &type->fs_supers);
spin_unlock(&sb_lock);
get_filesystem(type);
- register_shrinker(&s->s_shrink);
return s;
}
--
2.15.0
--
Michal Hocko
SUSE Labs
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v2] fs: handle shrinker registration failure in sget_userns
2017-11-23 14:35 ` [PATCH v2] " Michal Hocko
@ 2017-11-23 14:55 ` Al Viro
2017-11-23 15:02 ` Michal Hocko
` (2 more replies)
0 siblings, 3 replies; 28+ messages in thread
From: Al Viro @ 2017-11-23 14:55 UTC (permalink / raw)
To: Michal Hocko
Cc: Dave Chinner, Jan Kara, Tetsuo Handa, LKML, linux-fsdevel, Andrew Morton
On Thu, Nov 23, 2017 at 03:35:37PM +0100, Michal Hocko wrote:
> Hopefully less screwed version. But as I've said I am not really
> familiar with the code and do not feel competent to change it so please
> be very careful here. I've moved the shrinker registration to
> alloc_super which turned out to be simpler.
I don't get it. Why the hell do we need all that PITA in the first place?
Just make sget_userns() end with
if (unlikely(regsiter_shrinker(&s->s_shrink) != 0)) {
deactivate_locked_super(s);
s = ERR_PTR(-ENOMEM);
}
return s;
and be done with that. All there is to it...
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v2] fs: handle shrinker registration failure in sget_userns
2017-11-23 14:55 ` Al Viro
@ 2017-11-23 15:02 ` Michal Hocko
2017-11-23 15:34 ` Michal Hocko
2017-11-23 15:04 ` [PATCH v2] fs: handle shrinker registration failure insget_userns Tetsuo Handa
2017-11-29 11:55 ` Michal Hocko
2 siblings, 1 reply; 28+ messages in thread
From: Michal Hocko @ 2017-11-23 15:02 UTC (permalink / raw)
To: Al Viro
Cc: Dave Chinner, Jan Kara, Tetsuo Handa, LKML, linux-fsdevel, Andrew Morton
On Thu 23-11-17 14:55:40, Al Viro wrote:
> On Thu, Nov 23, 2017 at 03:35:37PM +0100, Michal Hocko wrote:
> > Hopefully less screwed version. But as I've said I am not really
> > familiar with the code and do not feel competent to change it so please
> > be very careful here. I've moved the shrinker registration to
> > alloc_super which turned out to be simpler.
>
> I don't get it. Why the hell do we need all that PITA in the first place?
> Just make sget_userns() end with
> if (unlikely(regsiter_shrinker(&s->s_shrink) != 0)) {
> deactivate_locked_super(s);
> s = ERR_PTR(-ENOMEM);
> }
> return s;
> and be done with that. All there is to it...
Who is going to unregister that shrinker on other failure paths?
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v2] fs: handle shrinker registration failure in sget_userns
2017-11-23 15:02 ` Michal Hocko
@ 2017-11-23 15:34 ` Michal Hocko
0 siblings, 0 replies; 28+ messages in thread
From: Michal Hocko @ 2017-11-23 15:34 UTC (permalink / raw)
To: Al Viro
Cc: Dave Chinner, Jan Kara, Tetsuo Handa, LKML, linux-fsdevel, Andrew Morton
On Thu 23-11-17 16:02:11, Michal Hocko wrote:
> On Thu 23-11-17 14:55:40, Al Viro wrote:
> > On Thu, Nov 23, 2017 at 03:35:37PM +0100, Michal Hocko wrote:
> > > Hopefully less screwed version. But as I've said I am not really
> > > familiar with the code and do not feel competent to change it so please
> > > be very careful here. I've moved the shrinker registration to
> > > alloc_super which turned out to be simpler.
> >
> > I don't get it. Why the hell do we need all that PITA in the first place?
> > Just make sget_userns() end with
> > if (unlikely(regsiter_shrinker(&s->s_shrink) != 0)) {
> > deactivate_locked_super(s);
> > s = ERR_PTR(-ENOMEM);
> > }
> > return s;
> > and be done with that. All there is to it...
>
> Who is going to unregister that shrinker on other failure paths?
Scratch that. I've mixed destroy_unused_super with
deactivate_locked_super. Go with whatever works...
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] fs: handle shrinker registration failure insget_userns
2017-11-23 14:55 ` Al Viro
2017-11-23 15:02 ` Michal Hocko
@ 2017-11-23 15:04 ` Tetsuo Handa
2017-11-23 15:28 ` Al Viro
2017-11-29 11:55 ` Michal Hocko
2 siblings, 1 reply; 28+ messages in thread
From: Tetsuo Handa @ 2017-11-23 15:04 UTC (permalink / raw)
To: viro, mhocko; +Cc: david, jack, linux-kernel, linux-fsdevel, akpm
Al Viro wrote:
> On Thu, Nov 23, 2017 at 03:35:37PM +0100, Michal Hocko wrote:
> > Hopefully less screwed version. But as I've said I am not really
> > familiar with the code and do not feel competent to change it so please
> > be very careful here. I've moved the shrinker registration to
> > alloc_super which turned out to be simpler.
>
> I don't get it. Why the hell do we need all that PITA in the first place?
> Just make sget_userns() end with
> if (unlikely(regsiter_shrinker(&s->s_shrink) != 0)) {
> deactivate_locked_super(s);
> s = ERR_PTR(-ENOMEM);
> }
> return s;
> and be done with that. All there is to it...
>
Doesn't deactivate_locked_super() call unregister_shrinker() ?
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v2] fs: handle shrinker registration failure insget_userns
2017-11-23 15:04 ` [PATCH v2] fs: handle shrinker registration failure insget_userns Tetsuo Handa
@ 2017-11-23 15:28 ` Al Viro
2017-11-23 15:35 ` [PATCH v2] fs: handle shrinker registration failure in sget_userns Tetsuo Handa
0 siblings, 1 reply; 28+ messages in thread
From: Al Viro @ 2017-11-23 15:28 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: mhocko, david, jack, linux-kernel, linux-fsdevel, akpm
On Fri, Nov 24, 2017 at 12:04:23AM +0900, Tetsuo Handa wrote:
> Al Viro wrote:
> > On Thu, Nov 23, 2017 at 03:35:37PM +0100, Michal Hocko wrote:
> > > Hopefully less screwed version. But as I've said I am not really
> > > familiar with the code and do not feel competent to change it so please
> > > be very careful here. I've moved the shrinker registration to
> > > alloc_super which turned out to be simpler.
> >
> > I don't get it. Why the hell do we need all that PITA in the first place?
> > Just make sget_userns() end with
> > if (unlikely(regsiter_shrinker(&s->s_shrink) != 0)) {
> > deactivate_locked_super(s);
> > s = ERR_PTR(-ENOMEM);
> > }
> > return s;
> > and be done with that. All there is to it...
> >
>
> Doesn't deactivate_locked_super() call unregister_shrinker() ?
And? unregister_shrinker() will do list_del() on empty list_head
and kfree(NULL); where's the problem with that?
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v2] fs: handle shrinker registration failure in sget_userns
2017-11-23 15:28 ` Al Viro
@ 2017-11-23 15:35 ` Tetsuo Handa
2017-11-23 15:44 ` Al Viro
0 siblings, 1 reply; 28+ messages in thread
From: Tetsuo Handa @ 2017-11-23 15:35 UTC (permalink / raw)
To: viro; +Cc: mhocko, david, jack, linux-kernel, linux-fsdevel, akpm
Al Viro wrote:
> On Fri, Nov 24, 2017 at 12:04:23AM +0900, Tetsuo Handa wrote:
> > Al Viro wrote:
> > > On Thu, Nov 23, 2017 at 03:35:37PM +0100, Michal Hocko wrote:
> > > > Hopefully less screwed version. But as I've said I am not really
> > > > familiar with the code and do not feel competent to change it so please
> > > > be very careful here. I've moved the shrinker registration to
> > > > alloc_super which turned out to be simpler.
> > >
> > > I don't get it. Why the hell do we need all that PITA in the first place?
> > > Just make sget_userns() end with
> > > if (unlikely(regsiter_shrinker(&s->s_shrink) != 0)) {
> > > deactivate_locked_super(s);
> > > s = ERR_PTR(-ENOMEM);
> > > }
> > > return s;
> > > and be done with that. All there is to it...
> > >
> >
> > Doesn't deactivate_locked_super() call unregister_shrinker() ?
>
> And? unregister_shrinker() will do list_del() on empty list_head
> and kfree(NULL); where's the problem with that?
>
The problem is that calling unregister_shrinker() without successful
register_shrinker() causes crash due to s_shrink.list.{prev,next} == NULL.
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v2] fs: handle shrinker registration failure in sget_userns
2017-11-23 15:35 ` [PATCH v2] fs: handle shrinker registration failure in sget_userns Tetsuo Handa
@ 2017-11-23 15:44 ` Al Viro
2017-11-23 21:51 ` Tetsuo Handa
0 siblings, 1 reply; 28+ messages in thread
From: Al Viro @ 2017-11-23 15:44 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: mhocko, david, jack, linux-kernel, linux-fsdevel, akpm
On Fri, Nov 24, 2017 at 12:35:29AM +0900, Tetsuo Handa wrote:
> Al Viro wrote:
> > On Fri, Nov 24, 2017 at 12:04:23AM +0900, Tetsuo Handa wrote:
> > > Al Viro wrote:
> > > > On Thu, Nov 23, 2017 at 03:35:37PM +0100, Michal Hocko wrote:
> > > > > Hopefully less screwed version. But as I've said I am not really
> > > > > familiar with the code and do not feel competent to change it so please
> > > > > be very careful here. I've moved the shrinker registration to
> > > > > alloc_super which turned out to be simpler.
> > > >
> > > > I don't get it. Why the hell do we need all that PITA in the first place?
> > > > Just make sget_userns() end with
> > > > if (unlikely(regsiter_shrinker(&s->s_shrink) != 0)) {
> > > > deactivate_locked_super(s);
> > > > s = ERR_PTR(-ENOMEM);
> > > > }
> > > > return s;
> > > > and be done with that. All there is to it...
> > > >
> > >
> > > Doesn't deactivate_locked_super() call unregister_shrinker() ?
> >
> > And? unregister_shrinker() will do list_del() on empty list_head
> > and kfree(NULL); where's the problem with that?
> >
> The problem is that calling unregister_shrinker() without successful
> register_shrinker() causes crash due to s_shrink.list.{prev,next} == NULL.
*shrug*
shrinker->nr_deferred = kzalloc(size, GFP_KERNEL);
if (!shrinker->nr_deferred) {
/* make sure it's in consistent state */
INIT_LIST_HEAD(&shrinker->list);
return -ENOMEM;
}
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v2] fs: handle shrinker registration failure in sget_userns
2017-11-23 15:44 ` Al Viro
@ 2017-11-23 21:51 ` Tetsuo Handa
2017-11-24 7:48 ` Michal Hocko
0 siblings, 1 reply; 28+ messages in thread
From: Tetsuo Handa @ 2017-11-23 21:51 UTC (permalink / raw)
To: mhocko; +Cc: viro, david, jack, linux-kernel, linux-fsdevel, akpm
Al Viro wrote:
> On Fri, Nov 24, 2017 at 12:35:29AM +0900, Tetsuo Handa wrote:
> > Al Viro wrote:
> > > On Fri, Nov 24, 2017 at 12:04:23AM +0900, Tetsuo Handa wrote:
> > > > Al Viro wrote:
> > > > > On Thu, Nov 23, 2017 at 03:35:37PM +0100, Michal Hocko wrote:
> > > > > > Hopefully less screwed version. But as I've said I am not really
> > > > > > familiar with the code and do not feel competent to change it so please
> > > > > > be very careful here. I've moved the shrinker registration to
> > > > > > alloc_super which turned out to be simpler.
> > > > >
> > > > > I don't get it. Why the hell do we need all that PITA in the first place?
> > > > > Just make sget_userns() end with
> > > > > if (unlikely(regsiter_shrinker(&s->s_shrink) != 0)) {
> > > > > deactivate_locked_super(s);
> > > > > s = ERR_PTR(-ENOMEM);
> > > > > }
> > > > > return s;
> > > > > and be done with that. All there is to it...
> > > > >
> > > >
> > > > Doesn't deactivate_locked_super() call unregister_shrinker() ?
> > >
> > > And? unregister_shrinker() will do list_del() on empty list_head
> > > and kfree(NULL); where's the problem with that?
> > >
> > The problem is that calling unregister_shrinker() without successful
> > register_shrinker() causes crash due to s_shrink.list.{prev,next} == NULL.
>
> *shrug*
> shrinker->nr_deferred = kzalloc(size, GFP_KERNEL);
> if (!shrinker->nr_deferred) {
> /* make sure it's in consistent state */
> INIT_LIST_HEAD(&shrinker->list);
> return -ENOMEM;
> }
>
>
Yes, that will work.
Michal, like Al thinks, making unregister_shrinker() no-op if
register_shrinker() failed simplifies things. Can we go with
http://lkml.kernel.org/r/1511265853-15654-1-git-send-email-penguin-kernel@I-love.SAKURA.ne.jp
with patch description updated to include Syzbot report?
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v2] fs: handle shrinker registration failure in sget_userns
2017-11-23 21:51 ` Tetsuo Handa
@ 2017-11-24 7:48 ` Michal Hocko
0 siblings, 0 replies; 28+ messages in thread
From: Michal Hocko @ 2017-11-24 7:48 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: viro, david, jack, linux-kernel, linux-fsdevel, akpm
On Fri 24-11-17 06:51:09, Tetsuo Handa wrote:
> Al Viro wrote:
> > On Fri, Nov 24, 2017 at 12:35:29AM +0900, Tetsuo Handa wrote:
> > > Al Viro wrote:
> > > > On Fri, Nov 24, 2017 at 12:04:23AM +0900, Tetsuo Handa wrote:
> > > > > Al Viro wrote:
> > > > > > On Thu, Nov 23, 2017 at 03:35:37PM +0100, Michal Hocko wrote:
> > > > > > > Hopefully less screwed version. But as I've said I am not really
> > > > > > > familiar with the code and do not feel competent to change it so please
> > > > > > > be very careful here. I've moved the shrinker registration to
> > > > > > > alloc_super which turned out to be simpler.
> > > > > >
> > > > > > I don't get it. Why the hell do we need all that PITA in the first place?
> > > > > > Just make sget_userns() end with
> > > > > > if (unlikely(regsiter_shrinker(&s->s_shrink) != 0)) {
> > > > > > deactivate_locked_super(s);
> > > > > > s = ERR_PTR(-ENOMEM);
> > > > > > }
> > > > > > return s;
> > > > > > and be done with that. All there is to it...
> > > > > >
> > > > >
> > > > > Doesn't deactivate_locked_super() call unregister_shrinker() ?
> > > >
> > > > And? unregister_shrinker() will do list_del() on empty list_head
> > > > and kfree(NULL); where's the problem with that?
> > > >
> > > The problem is that calling unregister_shrinker() without successful
> > > register_shrinker() causes crash due to s_shrink.list.{prev,next} == NULL.
> >
> > *shrug*
> > shrinker->nr_deferred = kzalloc(size, GFP_KERNEL);
> > if (!shrinker->nr_deferred) {
> > /* make sure it's in consistent state */
> > INIT_LIST_HEAD(&shrinker->list);
> > return -ENOMEM;
> > }
> >
> >
>
> Yes, that will work.
>
> Michal, like Al thinks, making unregister_shrinker() no-op if
> register_shrinker() failed simplifies things. Can we go with
> http://lkml.kernel.org/r/1511265853-15654-1-git-send-email-penguin-kernel@I-love.SAKURA.ne.jp
> with patch description updated to include Syzbot report?
I am not opposed to that patch. I just want it to make sure callers _do_
handle the error because a missing shrinker can cause memory pressure
realated issues. unregister_shrinker definitely shouldn't blow up but
I wanted it to warn. This would however trigger a false positive in this
path, right? It is true that the allocation failure would already
trigger warning so the clean up path could be more relaxed. It can be
still quite some time between those two events.
In any case. I do not have a strong preference. If relying on
deactivate_locked_super is really seem much better for the vfs code then
let's go with your patch without warning.
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] fs: handle shrinker registration failure in sget_userns
2017-11-23 14:55 ` Al Viro
2017-11-23 15:02 ` Michal Hocko
2017-11-23 15:04 ` [PATCH v2] fs: handle shrinker registration failure insget_userns Tetsuo Handa
@ 2017-11-29 11:55 ` Michal Hocko
2017-12-09 20:59 ` Al Viro
2 siblings, 1 reply; 28+ messages in thread
From: Michal Hocko @ 2017-11-29 11:55 UTC (permalink / raw)
To: Al Viro
Cc: Dave Chinner, Jan Kara, Tetsuo Handa, LKML, linux-fsdevel, Andrew Morton
On Thu 23-11-17 14:55:40, Al Viro wrote:
> On Thu, Nov 23, 2017 at 03:35:37PM +0100, Michal Hocko wrote:
> > Hopefully less screwed version. But as I've said I am not really
> > familiar with the code and do not feel competent to change it so please
> > be very careful here. I've moved the shrinker registration to
> > alloc_super which turned out to be simpler.
>
> I don't get it. Why the hell do we need all that PITA in the first place?
> Just make sget_userns() end with
> if (unlikely(regsiter_shrinker(&s->s_shrink) != 0)) {
> deactivate_locked_super(s);
> s = ERR_PTR(-ENOMEM);
> }
> return s;
> and be done with that. All there is to it...
Al, do you plan to push this fix? Tetsuo's unregister_shrinker
fortification is already in the mmotm tree.
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v2] fs: handle shrinker registration failure in sget_userns
2017-11-29 11:55 ` Michal Hocko
@ 2017-12-09 20:59 ` Al Viro
2017-12-09 21:54 ` Al Viro
0 siblings, 1 reply; 28+ messages in thread
From: Al Viro @ 2017-12-09 20:59 UTC (permalink / raw)
To: Michal Hocko
Cc: Dave Chinner, Jan Kara, Tetsuo Handa, LKML, linux-fsdevel, Andrew Morton
On Wed, Nov 29, 2017 at 12:55:15PM +0100, Michal Hocko wrote:
> On Thu 23-11-17 14:55:40, Al Viro wrote:
> > On Thu, Nov 23, 2017 at 03:35:37PM +0100, Michal Hocko wrote:
> > > Hopefully less screwed version. But as I've said I am not really
> > > familiar with the code and do not feel competent to change it so please
> > > be very careful here. I've moved the shrinker registration to
> > > alloc_super which turned out to be simpler.
> >
> > I don't get it. Why the hell do we need all that PITA in the first place?
> > Just make sget_userns() end with
> > if (unlikely(regsiter_shrinker(&s->s_shrink) != 0)) {
> > deactivate_locked_super(s);
> > s = ERR_PTR(-ENOMEM);
> > }
> > return s;
> > and be done with that. All there is to it...
>
> Al, do you plan to push this fix? Tetsuo's unregister_shrinker
> fortification is already in the mmotm tree.
Is it in any git branch I could pull from? Or I could just throw it
into vfs.git#for-linus before the fix above - up to you, folks...
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v2] fs: handle shrinker registration failure in sget_userns
2017-12-09 20:59 ` Al Viro
@ 2017-12-09 21:54 ` Al Viro
2017-12-10 2:33 ` Tetsuo Handa
0 siblings, 1 reply; 28+ messages in thread
From: Al Viro @ 2017-12-09 21:54 UTC (permalink / raw)
To: Michal Hocko
Cc: Dave Chinner, Jan Kara, Tetsuo Handa, LKML, linux-fsdevel, Andrew Morton
On Sat, Dec 09, 2017 at 08:59:22PM +0000, Al Viro wrote:
> On Wed, Nov 29, 2017 at 12:55:15PM +0100, Michal Hocko wrote:
> > On Thu 23-11-17 14:55:40, Al Viro wrote:
> > > On Thu, Nov 23, 2017 at 03:35:37PM +0100, Michal Hocko wrote:
> > > > Hopefully less screwed version. But as I've said I am not really
> > > > familiar with the code and do not feel competent to change it so please
> > > > be very careful here. I've moved the shrinker registration to
> > > > alloc_super which turned out to be simpler.
> > >
> > > I don't get it. Why the hell do we need all that PITA in the first place?
> > > Just make sget_userns() end with
> > > if (unlikely(regsiter_shrinker(&s->s_shrink) != 0)) {
> > > deactivate_locked_super(s);
> > > s = ERR_PTR(-ENOMEM);
> > > }
> > > return s;
> > > and be done with that. All there is to it...
> >
> > Al, do you plan to push this fix? Tetsuo's unregister_shrinker
> > fortification is already in the mmotm tree.
>
> Is it in any git branch I could pull from? Or I could just throw it
> into vfs.git#for-linus before the fix above - up to you, folks...
Actually, looking at mmotm... I don't see it there. Which patch
is it in?
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v2] fs: handle shrinker registration failure in sget_userns
2017-12-09 21:54 ` Al Viro
@ 2017-12-10 2:33 ` Tetsuo Handa
2017-12-10 10:05 ` Michal Hocko
0 siblings, 1 reply; 28+ messages in thread
From: Tetsuo Handa @ 2017-12-10 2:33 UTC (permalink / raw)
To: viro, mhocko; +Cc: david, jack, linux-kernel, linux-fsdevel, akpm
Al Viro wrote:
> On Sat, Dec 09, 2017 at 08:59:22PM +0000, Al Viro wrote:
> > On Wed, Nov 29, 2017 at 12:55:15PM +0100, Michal Hocko wrote:
> > > On Thu 23-11-17 14:55:40, Al Viro wrote:
> > > > On Thu, Nov 23, 2017 at 03:35:37PM +0100, Michal Hocko wrote:
> > > > > Hopefully less screwed version. But as I've said I am not really
> > > > > familiar with the code and do not feel competent to change it so please
> > > > > be very careful here. I've moved the shrinker registration to
> > > > > alloc_super which turned out to be simpler.
> > > >
> > > > I don't get it. Why the hell do we need all that PITA in the first place?
> > > > Just make sget_userns() end with
> > > > if (unlikely(regsiter_shrinker(&s->s_shrink) != 0)) {
> > > > deactivate_locked_super(s);
> > > > s = ERR_PTR(-ENOMEM);
> > > > }
> > > > return s;
> > > > and be done with that. All there is to it...
> > >
> > > Al, do you plan to push this fix? Tetsuo's unregister_shrinker
> > > fortification is already in the mmotm tree.
> >
> > Is it in any git branch I could pull from? Or I could just throw it
> > into vfs.git#for-linus before the fix above - up to you, folks...
>
> Actually, looking at mmotm... I don't see it there. Which patch
> is it in?
>
My unregister_shrinker() fortification patch
( http://lkml.kernel.org/r/1511523385-6433-1-git-send-email-penguin-kernel@I-love.SAKURA.ne.jp )
is not yet in the mmotm tree due to disagreement between Michal and I, but
you can throw your sget_userns() patch into vfs.git#for-linus anyway.
We will eventually apply unregister_shrinker() fortification patch.
I'm observing whether people notice my __must_check annotation patch
( http://lkml.kernel.org/r/1511523385-6433-2-git-send-email-penguin-kernel@I-love.SAKURA.ne.jp )
in linux-next.git and fix register_shrinker() callers. If people do not fix
the callers, both patches need to be sent to linux.git.
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v2] fs: handle shrinker registration failure in sget_userns
2017-12-10 2:33 ` Tetsuo Handa
@ 2017-12-10 10:05 ` Michal Hocko
2017-12-10 15:21 ` Al Viro
0 siblings, 1 reply; 28+ messages in thread
From: Michal Hocko @ 2017-12-10 10:05 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: viro, david, jack, linux-kernel, linux-fsdevel, akpm
On Sun 10-12-17 11:33:18, Tetsuo Handa wrote:
> Al Viro wrote:
> > On Sat, Dec 09, 2017 at 08:59:22PM +0000, Al Viro wrote:
> > > On Wed, Nov 29, 2017 at 12:55:15PM +0100, Michal Hocko wrote:
> > > > On Thu 23-11-17 14:55:40, Al Viro wrote:
> > > > > On Thu, Nov 23, 2017 at 03:35:37PM +0100, Michal Hocko wrote:
> > > > > > Hopefully less screwed version. But as I've said I am not really
> > > > > > familiar with the code and do not feel competent to change it so please
> > > > > > be very careful here. I've moved the shrinker registration to
> > > > > > alloc_super which turned out to be simpler.
> > > > >
> > > > > I don't get it. Why the hell do we need all that PITA in the first place?
> > > > > Just make sget_userns() end with
> > > > > if (unlikely(regsiter_shrinker(&s->s_shrink) != 0)) {
> > > > > deactivate_locked_super(s);
> > > > > s = ERR_PTR(-ENOMEM);
> > > > > }
> > > > > return s;
> > > > > and be done with that. All there is to it...
> > > >
> > > > Al, do you plan to push this fix? Tetsuo's unregister_shrinker
> > > > fortification is already in the mmotm tree.
> > >
> > > Is it in any git branch I could pull from? Or I could just throw it
> > > into vfs.git#for-linus before the fix above - up to you, folks...
> >
> > Actually, looking at mmotm... I don't see it there. Which patch
> > is it in?
> >
>
> My unregister_shrinker() fortification patch
> ( http://lkml.kernel.org/r/1511523385-6433-1-git-send-email-penguin-kernel@I-love.SAKURA.ne.jp )
> is not yet in the mmotm tree due to disagreement between Michal and I, but
> you can throw your sget_userns() patch into vfs.git#for-linus anyway.
> We will eventually apply unregister_shrinker() fortification patch.
I've acked the patch http://lkml.kernel.org/r/20171124122148.qevmiogh3pzr4zix@dhcp22.suse.cz
I disagreed with your must_check patch which has nothing to do with the
patch disussed here. I've also suggested some changelog clarifications.
Please repost and I am pretty sure Andew will pick it up.
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v2] fs: handle shrinker registration failure in sget_userns
2017-12-10 10:05 ` Michal Hocko
@ 2017-12-10 15:21 ` Al Viro
0 siblings, 0 replies; 28+ messages in thread
From: Al Viro @ 2017-12-10 15:21 UTC (permalink / raw)
To: Michal Hocko; +Cc: Tetsuo Handa, david, jack, linux-kernel, linux-fsdevel, akpm
On Sun, Dec 10, 2017 at 11:05:15AM +0100, Michal Hocko wrote:
> > My unregister_shrinker() fortification patch
> > ( http://lkml.kernel.org/r/1511523385-6433-1-git-send-email-penguin-kernel@I-love.SAKURA.ne.jp )
> > is not yet in the mmotm tree due to disagreement between Michal and I, but
> > you can throw your sget_userns() patch into vfs.git#for-linus anyway.
> > We will eventually apply unregister_shrinker() fortification patch.
>
> I've acked the patch http://lkml.kernel.org/r/20171124122148.qevmiogh3pzr4zix@dhcp22.suse.cz
> I disagreed with your must_check patch which has nothing to do with the
> patch disussed here. I've also suggested some changelog clarifications.
> Please repost and I am pretty sure Andew will pick it up.
Hell, I will pick it up, add sget one on top of that and put both into #for-linus.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] fs: handle shrinker registration failure in sget_userns
2017-11-23 11:52 [PATCH] fs: handle shrinker registration failure in sget_userns Michal Hocko
2017-11-23 12:26 ` Jan Kara
2017-11-23 14:35 ` [PATCH v2] " Michal Hocko
@ 2017-11-23 14:47 ` Al Viro
2017-11-23 15:00 ` Michal Hocko
2 siblings, 1 reply; 28+ messages in thread
From: Al Viro @ 2017-11-23 14:47 UTC (permalink / raw)
To: Michal Hocko
Cc: Dave Chinner, Jan Kara, Tetsuo Handa, LKML, linux-fsdevel,
Andrew Morton, Michal Hocko
On Thu, Nov 23, 2017 at 12:52:47PM +0100, Michal Hocko wrote:
> @@ -503,12 +504,18 @@ struct super_block *sget_userns(struct file_system_type *type,
> s = alloc_super(type, (flags & ~SB_SUBMOUNT), user_ns);
> if (!s)
> return ERR_PTR(-ENOMEM);
> + if (register_shrinker(&s->s_shrink)) {
> + spin_unlock(&sb_lock);
> + destroy_unused_super(s);
> + return ERR_PTR(-ENOMEM);
Have you even tested that? What the hell is that spin_unlock() doing there?
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH] fs: handle shrinker registration failure in sget_userns
2017-11-23 14:47 ` [PATCH] " Al Viro
@ 2017-11-23 15:00 ` Michal Hocko
0 siblings, 0 replies; 28+ messages in thread
From: Michal Hocko @ 2017-11-23 15:00 UTC (permalink / raw)
To: Al Viro
Cc: Dave Chinner, Jan Kara, Tetsuo Handa, LKML, linux-fsdevel, Andrew Morton
On Thu 23-11-17 14:47:35, Al Viro wrote:
> On Thu, Nov 23, 2017 at 12:52:47PM +0100, Michal Hocko wrote:
> > @@ -503,12 +504,18 @@ struct super_block *sget_userns(struct file_system_type *type,
> > s = alloc_super(type, (flags & ~SB_SUBMOUNT), user_ns);
> > if (!s)
> > return ERR_PTR(-ENOMEM);
> > + if (register_shrinker(&s->s_shrink)) {
> > + spin_unlock(&sb_lock);
> > + destroy_unused_super(s);
> > + return ERR_PTR(-ENOMEM);
>
> Have you even tested that? What the hell is that spin_unlock() doing there?
Yeah, that patch was just screwed up. Have a look at
http://lkml.kernel.org/r/20171123143537.ztpxpk3sjbpo72rf@dhcp22.suse.cz
which should even resemble a real patch.
Sorry about all the confusion.
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2017-12-10 15:22 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-23 11:52 [PATCH] fs: handle shrinker registration failure in sget_userns Michal Hocko
2017-11-23 12:26 ` Jan Kara
2017-11-23 12:45 ` Michal Hocko
2017-11-23 12:53 ` Michal Hocko
2017-11-23 12:56 ` Jan Kara
2017-11-23 13:35 ` Tetsuo Handa
2017-11-23 13:46 ` Michal Hocko
2017-11-23 13:57 ` Tetsuo Handa
2017-11-23 14:06 ` Michal Hocko
2017-11-23 14:10 ` Tetsuo Handa
2017-11-23 14:35 ` [PATCH v2] " Michal Hocko
2017-11-23 14:55 ` Al Viro
2017-11-23 15:02 ` Michal Hocko
2017-11-23 15:34 ` Michal Hocko
2017-11-23 15:04 ` [PATCH v2] fs: handle shrinker registration failure insget_userns Tetsuo Handa
2017-11-23 15:28 ` Al Viro
2017-11-23 15:35 ` [PATCH v2] fs: handle shrinker registration failure in sget_userns Tetsuo Handa
2017-11-23 15:44 ` Al Viro
2017-11-23 21:51 ` Tetsuo Handa
2017-11-24 7:48 ` Michal Hocko
2017-11-29 11:55 ` Michal Hocko
2017-12-09 20:59 ` Al Viro
2017-12-09 21:54 ` Al Viro
2017-12-10 2:33 ` Tetsuo Handa
2017-12-10 10:05 ` Michal Hocko
2017-12-10 15:21 ` Al Viro
2017-11-23 14:47 ` [PATCH] " Al Viro
2017-11-23 15:00 ` Michal Hocko
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.