All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 1/2] mm,vmscan: Make unregister_shrinker() no-op if register_shrinker() failed.
@ 2017-11-24 11:36 Tetsuo Handa
  2017-11-24 11:36 ` [PATCH v2 2/2] mm,vmscan: Mark register_shrinker() as __must_check Tetsuo Handa
  2017-11-24 12:21 ` [PATCH v2 1/2] mm,vmscan: Make unregister_shrinker() no-op if register_shrinker() failed Michal Hocko
  0 siblings, 2 replies; 12+ messages in thread
From: Tetsuo Handa @ 2017-11-24 11:36 UTC (permalink / raw)
  To: linux-mm; +Cc: akpm, mhocko, Tetsuo Handa, Al Viro, Glauber Costa, syzbot

Syzbot caught an oops at unregister_shrinker() because combination of
commit 1d3d4437eae1bb29 ("vmscan: per-node deferred work") and fault
injection made register_shrinker() fail and the caller of
register_shrinker() did not check for failure.

----------
[  554.881422] FAULT_INJECTION: forcing a failure.
[  554.881422] name failslab, interval 1, probability 0, space 0, times 0
[  554.881438] CPU: 1 PID: 13231 Comm: syz-executor1 Not tainted 4.14.0-rc8+ #82
[  554.881443] Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
[  554.881445] Call Trace:
[  554.881459]  dump_stack+0x194/0x257
[  554.881474]  ? arch_local_irq_restore+0x53/0x53
[  554.881486]  ? find_held_lock+0x35/0x1d0
[  554.881507]  should_fail+0x8c0/0xa40
[  554.881522]  ? fault_create_debugfs_attr+0x1f0/0x1f0
[  554.881537]  ? check_noncircular+0x20/0x20
[  554.881546]  ? find_next_zero_bit+0x2c/0x40
[  554.881560]  ? ida_get_new_above+0x421/0x9d0
[  554.881577]  ? find_held_lock+0x35/0x1d0
[  554.881594]  ? __lock_is_held+0xb6/0x140
[  554.881628]  ? check_same_owner+0x320/0x320
[  554.881634]  ? lock_downgrade+0x990/0x990
[  554.881649]  ? find_held_lock+0x35/0x1d0
[  554.881672]  should_failslab+0xec/0x120
[  554.881684]  __kmalloc+0x63/0x760
[  554.881692]  ? lock_downgrade+0x990/0x990
[  554.881712]  ? register_shrinker+0x10e/0x2d0
[  554.881721]  ? trace_event_raw_event_module_request+0x320/0x320
[  554.881737]  register_shrinker+0x10e/0x2d0
[  554.881747]  ? prepare_kswapd_sleep+0x1f0/0x1f0
[  554.881755]  ? _down_write_nest_lock+0x120/0x120
[  554.881765]  ? memcpy+0x45/0x50
[  554.881785]  sget_userns+0xbcd/0xe20
(...snipped...)
[  554.898693] kasan: CONFIG_KASAN_INLINE enabled
[  554.898724] kasan: GPF could be caused by NULL-ptr deref or user memory access
[  554.898732] general protection fault: 0000 [#1] SMP KASAN
[  554.898737] Dumping ftrace buffer:
[  554.898741]    (ftrace buffer empty)
[  554.898743] Modules linked in:
[  554.898752] CPU: 1 PID: 13231 Comm: syz-executor1 Not tainted 4.14.0-rc8+ #82
[  554.898755] Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
[  554.898760] task: ffff8801d1dbe5c0 task.stack: ffff8801c9e38000
[  554.898772] RIP: 0010:__list_del_entry_valid+0x7e/0x150
[  554.898775] RSP: 0018:ffff8801c9e3f108 EFLAGS: 00010246
[  554.898780] RAX: dffffc0000000000 RBX: 0000000000000000 RCX: 0000000000000000
[  554.898784] RDX: 0000000000000000 RSI: ffff8801c53c6f98 RDI: ffff8801c53c6fa0
[  554.898788] RBP: ffff8801c9e3f120 R08: 1ffff100393c7d55 R09: 0000000000000004
[  554.898791] R10: ffff8801c9e3ef70 R11: 0000000000000000 R12: 0000000000000000
[  554.898795] R13: dffffc0000000000 R14: 1ffff100393c7e45 R15: ffff8801c53c6f98
[  554.898800] FS:  0000000000000000(0000) GS:ffff8801db300000(0000) knlGS:0000000000000000
[  554.898804] CS:  0010 DS: 002b ES: 002b CR0: 0000000080050033
[  554.898807] CR2: 00000000dbc23000 CR3: 00000001c7269000 CR4: 00000000001406e0
[  554.898813] DR0: 0000000020000000 DR1: 0000000020000000 DR2: 0000000000000000
[  554.898816] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000600
[  554.898818] Call Trace:
[  554.898828]  unregister_shrinker+0x79/0x300
[  554.898837]  ? perf_trace_mm_vmscan_writepage+0x750/0x750
[  554.898844]  ? down_write+0x87/0x120
[  554.898851]  ? deactivate_super+0x139/0x1b0
[  554.898857]  ? down_read+0x150/0x150
[  554.898864]  ? check_same_owner+0x320/0x320
[  554.898875]  deactivate_locked_super+0x64/0xd0
[  554.898883]  deactivate_super+0x141/0x1b0
----------

Since allowing register_shrinker() callers to call unregister_shrinker()
when register_shrinker() failed can simplify error recovery path, this
patch makes unregister_shrinker() no-op when register_shrinker() failed.
Since we can encourage register_shrinker() callers to check for failure
by marking register_shrinker() as __must_check, unregister_shrinker()
can stay silent.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Reported-by: syzbot <syzkaller@googlegroups.com>
Cc: Glauber Costa <glauber@scylladb.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
---
 mm/vmscan.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 6a5a72b..d01177b 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -297,6 +297,8 @@ int register_shrinker(struct shrinker *shrinker)
  */
 void unregister_shrinker(struct shrinker *shrinker)
 {
+	if (!shrinker->nr_deferred)
+		return;
 	down_write(&shrinker_rwsem);
 	list_del(&shrinker->list);
 	up_write(&shrinker_rwsem);
-- 
1.8.3.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 2/2] mm,vmscan: Mark register_shrinker() as __must_check
  2017-11-24 11:36 [PATCH v2 1/2] mm,vmscan: Make unregister_shrinker() no-op if register_shrinker() failed Tetsuo Handa
@ 2017-11-24 11:36 ` Tetsuo Handa
  2017-11-24 12:24   ` Michal Hocko
  2017-11-24 12:21 ` [PATCH v2 1/2] mm,vmscan: Make unregister_shrinker() no-op if register_shrinker() failed Michal Hocko
  1 sibling, 1 reply; 12+ messages in thread
From: Tetsuo Handa @ 2017-11-24 11:36 UTC (permalink / raw)
  To: linux-mm; +Cc: akpm, mhocko, Tetsuo Handa, Al Viro, Glauber Costa

Commit 1d3d4437eae1bb29 ("vmscan: per-node deferred work") changed
register_shrinker() to fail when memory allocation failed.
Since that commit did not take appropriate precautions before allowing
register_shrinker() to fail, there are many register_shrinker() users
who continue running when register_shrinker() failed.
Since continuing when register_shrinker() failed can cause memory
pressure related issues (e.g. needless OOM killer invocations),
this patch marks register_shrinker() as __must_check in order to
encourage all register_shrinker() users to add error recovery path.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Glauber Costa <glauber@scylladb.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
---
 include/linux/shrinker.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h
index 388ff29..a389491 100644
--- a/include/linux/shrinker.h
+++ b/include/linux/shrinker.h
@@ -75,6 +75,6 @@ struct shrinker {
 #define SHRINKER_NUMA_AWARE	(1 << 0)
 #define SHRINKER_MEMCG_AWARE	(1 << 1)
 
-extern int register_shrinker(struct shrinker *);
+extern __must_check int register_shrinker(struct shrinker *);
 extern void unregister_shrinker(struct shrinker *);
 #endif
-- 
1.8.3.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/2] mm,vmscan: Make unregister_shrinker() no-op if register_shrinker() failed.
  2017-11-24 11:36 [PATCH v2 1/2] mm,vmscan: Make unregister_shrinker() no-op if register_shrinker() failed Tetsuo Handa
  2017-11-24 11:36 ` [PATCH v2 2/2] mm,vmscan: Mark register_shrinker() as __must_check Tetsuo Handa
@ 2017-11-24 12:21 ` Michal Hocko
  2017-11-24 13:21   ` Tetsuo Handa
  1 sibling, 1 reply; 12+ messages in thread
From: Michal Hocko @ 2017-11-24 12:21 UTC (permalink / raw)
  To: Tetsuo Handa; +Cc: linux-mm, akpm, Al Viro, Glauber Costa, syzbot

On Fri 24-11-17 20:36:24, Tetsuo Handa wrote:
> Syzbot caught an oops at unregister_shrinker() because combination of
> commit 1d3d4437eae1bb29 ("vmscan: per-node deferred work") and fault
> injection made register_shrinker() fail and the caller of
> register_shrinker() did not check for failure.
> 
> ----------
> [  554.881422] FAULT_INJECTION: forcing a failure.
> [  554.881422] name failslab, interval 1, probability 0, space 0, times 0
> [  554.881438] CPU: 1 PID: 13231 Comm: syz-executor1 Not tainted 4.14.0-rc8+ #82
> [  554.881443] Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> [  554.881445] Call Trace:
> [  554.881459]  dump_stack+0x194/0x257
> [  554.881474]  ? arch_local_irq_restore+0x53/0x53
> [  554.881486]  ? find_held_lock+0x35/0x1d0
> [  554.881507]  should_fail+0x8c0/0xa40
> [  554.881522]  ? fault_create_debugfs_attr+0x1f0/0x1f0
> [  554.881537]  ? check_noncircular+0x20/0x20
> [  554.881546]  ? find_next_zero_bit+0x2c/0x40
> [  554.881560]  ? ida_get_new_above+0x421/0x9d0
> [  554.881577]  ? find_held_lock+0x35/0x1d0
> [  554.881594]  ? __lock_is_held+0xb6/0x140
> [  554.881628]  ? check_same_owner+0x320/0x320
> [  554.881634]  ? lock_downgrade+0x990/0x990
> [  554.881649]  ? find_held_lock+0x35/0x1d0
> [  554.881672]  should_failslab+0xec/0x120
> [  554.881684]  __kmalloc+0x63/0x760
> [  554.881692]  ? lock_downgrade+0x990/0x990
> [  554.881712]  ? register_shrinker+0x10e/0x2d0
> [  554.881721]  ? trace_event_raw_event_module_request+0x320/0x320
> [  554.881737]  register_shrinker+0x10e/0x2d0
> [  554.881747]  ? prepare_kswapd_sleep+0x1f0/0x1f0
> [  554.881755]  ? _down_write_nest_lock+0x120/0x120
> [  554.881765]  ? memcpy+0x45/0x50
> [  554.881785]  sget_userns+0xbcd/0xe20
> (...snipped...)
> [  554.898693] kasan: CONFIG_KASAN_INLINE enabled
> [  554.898724] kasan: GPF could be caused by NULL-ptr deref or user memory access
> [  554.898732] general protection fault: 0000 [#1] SMP KASAN
> [  554.898737] Dumping ftrace buffer:
> [  554.898741]    (ftrace buffer empty)
> [  554.898743] Modules linked in:
> [  554.898752] CPU: 1 PID: 13231 Comm: syz-executor1 Not tainted 4.14.0-rc8+ #82
> [  554.898755] Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> [  554.898760] task: ffff8801d1dbe5c0 task.stack: ffff8801c9e38000
> [  554.898772] RIP: 0010:__list_del_entry_valid+0x7e/0x150
> [  554.898775] RSP: 0018:ffff8801c9e3f108 EFLAGS: 00010246
> [  554.898780] RAX: dffffc0000000000 RBX: 0000000000000000 RCX: 0000000000000000
> [  554.898784] RDX: 0000000000000000 RSI: ffff8801c53c6f98 RDI: ffff8801c53c6fa0
> [  554.898788] RBP: ffff8801c9e3f120 R08: 1ffff100393c7d55 R09: 0000000000000004
> [  554.898791] R10: ffff8801c9e3ef70 R11: 0000000000000000 R12: 0000000000000000
> [  554.898795] R13: dffffc0000000000 R14: 1ffff100393c7e45 R15: ffff8801c53c6f98
> [  554.898800] FS:  0000000000000000(0000) GS:ffff8801db300000(0000) knlGS:0000000000000000
> [  554.898804] CS:  0010 DS: 002b ES: 002b CR0: 0000000080050033
> [  554.898807] CR2: 00000000dbc23000 CR3: 00000001c7269000 CR4: 00000000001406e0
> [  554.898813] DR0: 0000000020000000 DR1: 0000000020000000 DR2: 0000000000000000
> [  554.898816] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000600
> [  554.898818] Call Trace:
> [  554.898828]  unregister_shrinker+0x79/0x300
> [  554.898837]  ? perf_trace_mm_vmscan_writepage+0x750/0x750
> [  554.898844]  ? down_write+0x87/0x120
> [  554.898851]  ? deactivate_super+0x139/0x1b0
> [  554.898857]  ? down_read+0x150/0x150
> [  554.898864]  ? check_same_owner+0x320/0x320
> [  554.898875]  deactivate_locked_super+0x64/0xd0
> [  554.898883]  deactivate_super+0x141/0x1b0
> ----------
> 
> Since allowing register_shrinker() callers to call unregister_shrinker()
> when register_shrinker() failed can simplify error recovery path, this
> patch makes unregister_shrinker() no-op when register_shrinker() failed.

Well, the primary point of this patch is to not blow up, in the first
place. It is not to woraround missing register_shrinker handling which
what one could understand from the above. Yes we are making it noop
because it is easier for users but they still _have_ to check the error
path otherwise we have silent memory pressure issues potentially.

> Since we can encourage register_shrinker() callers to check for failure
> by marking register_shrinker() as __must_check, unregister_shrinker()
> can stay silent.

I am not sure __must_check is the right way. We already do get
allocation warning if the registration fails so silent unregister is
acceptable. Unchecked register_shrinker is a bug like any other
unchecked error path.
 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Reported-by: syzbot <syzkaller@googlegroups.com>
> Cc: Glauber Costa <glauber@scylladb.com>
> Cc: Al Viro <viro@zeniv.linux.org.uk>

With the changelog updated
Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  mm/vmscan.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 6a5a72b..d01177b 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -297,6 +297,8 @@ int register_shrinker(struct shrinker *shrinker)
>   */
>  void unregister_shrinker(struct shrinker *shrinker)
>  {
> +	if (!shrinker->nr_deferred)
> +		return;
>  	down_write(&shrinker_rwsem);
>  	list_del(&shrinker->list);
>  	up_write(&shrinker_rwsem);
> -- 
> 1.8.3.1
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 2/2] mm,vmscan: Mark register_shrinker() as __must_check
  2017-11-24 11:36 ` [PATCH v2 2/2] mm,vmscan: Mark register_shrinker() as __must_check Tetsuo Handa
@ 2017-11-24 12:24   ` Michal Hocko
  2017-11-24 13:26     ` Tetsuo Handa
  0 siblings, 1 reply; 12+ messages in thread
From: Michal Hocko @ 2017-11-24 12:24 UTC (permalink / raw)
  To: Tetsuo Handa; +Cc: linux-mm, akpm, Al Viro, Glauber Costa

On Fri 24-11-17 20:36:25, Tetsuo Handa wrote:
> Commit 1d3d4437eae1bb29 ("vmscan: per-node deferred work") changed
> register_shrinker() to fail when memory allocation failed.
> Since that commit did not take appropriate precautions before allowing
> register_shrinker() to fail, there are many register_shrinker() users
> who continue running when register_shrinker() failed.
> Since continuing when register_shrinker() failed can cause memory
> pressure related issues (e.g. needless OOM killer invocations),
> this patch marks register_shrinker() as __must_check in order to
> encourage all register_shrinker() users to add error recovery path.
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Cc: Glauber Costa <glauber@scylladb.com>
> Cc: Al Viro <viro@zeniv.linux.org.uk>

As already pointed out, I do not think this is worth it. This function
is no different than many others which need error handling. The system
will work suboptimally when the shrinker is missing, no question about
that, but there is no immediate blow up otherwise. It is not all that
hard to find all those places and fix them up. We do not have hundreds
of them...

> ---
>  include/linux/shrinker.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h
> index 388ff29..a389491 100644
> --- a/include/linux/shrinker.h
> +++ b/include/linux/shrinker.h
> @@ -75,6 +75,6 @@ struct shrinker {
>  #define SHRINKER_NUMA_AWARE	(1 << 0)
>  #define SHRINKER_MEMCG_AWARE	(1 << 1)
>  
> -extern int register_shrinker(struct shrinker *);
> +extern __must_check int register_shrinker(struct shrinker *);
>  extern void unregister_shrinker(struct shrinker *);
>  #endif
> -- 
> 1.8.3.1

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/2] mm,vmscan: Make unregister_shrinker() no-op if register_shrinker() failed.
  2017-11-24 12:21 ` [PATCH v2 1/2] mm,vmscan: Make unregister_shrinker() no-op if register_shrinker() failed Michal Hocko
@ 2017-11-24 13:21   ` Tetsuo Handa
  2017-11-24 13:28     ` Michal Hocko
  0 siblings, 1 reply; 12+ messages in thread
From: Tetsuo Handa @ 2017-11-24 13:21 UTC (permalink / raw)
  To: mhocko; +Cc: linux-mm, akpm, viro, glauber, syzkaller

Michal Hocko wrote:
> > Since we can encourage register_shrinker() callers to check for failure
> > by marking register_shrinker() as __must_check, unregister_shrinker()
> > can stay silent.
> 
> I am not sure __must_check is the right way. We already do get
> allocation warning if the registration fails so silent unregister is
> acceptable. Unchecked register_shrinker is a bug like any other
> unchecked error path.

I consider that __must_check is the simplest way to find all of
unchecked register_shrinker bugs. Why not to encourage users to fix?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 2/2] mm,vmscan: Mark register_shrinker() as __must_check
  2017-11-24 12:24   ` Michal Hocko
@ 2017-11-24 13:26     ` Tetsuo Handa
  2017-11-24 13:37       ` Michal Hocko
  0 siblings, 1 reply; 12+ messages in thread
From: Tetsuo Handa @ 2017-11-24 13:26 UTC (permalink / raw)
  To: mhocko; +Cc: linux-mm, akpm, viro, glauber

Michal Hocko wrote:
> As already pointed out, I do not think this is worth it. This function
> is no different than many others which need error handling. The system
> will work suboptimally when the shrinker is missing, no question about
> that, but there is no immediate blow up otherwise. It is not all that
> hard to find all those places and fix them up. We do not have hundreds
> of them...

The system might blow up after two years of uptime. For enterprise systems
which try to avoid reboots as much as possible, it is important to fix
known bugs before deploying.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/2] mm,vmscan: Make unregister_shrinker() no-op if register_shrinker() failed.
  2017-11-24 13:21   ` Tetsuo Handa
@ 2017-11-24 13:28     ` Michal Hocko
  2017-11-25  1:40       ` Tetsuo Handa
  0 siblings, 1 reply; 12+ messages in thread
From: Michal Hocko @ 2017-11-24 13:28 UTC (permalink / raw)
  To: Tetsuo Handa; +Cc: linux-mm, akpm, viro, glauber, syzkaller

On Fri 24-11-17 22:21:55, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > > Since we can encourage register_shrinker() callers to check for failure
> > > by marking register_shrinker() as __must_check, unregister_shrinker()
> > > can stay silent.
> > 
> > I am not sure __must_check is the right way. We already do get
> > allocation warning if the registration fails so silent unregister is
> > acceptable. Unchecked register_shrinker is a bug like any other
> > unchecked error path.
> 
> I consider that __must_check is the simplest way to find all of
> unchecked register_shrinker bugs. Why not to encourage users to fix?

because git grep doesn't require to patch the kernel and still provide
the information you want. I would understand __must_check if we had
hundreds users of this api and they come and go quickly.

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 2/2] mm,vmscan: Mark register_shrinker() as __must_check
  2017-11-24 13:26     ` Tetsuo Handa
@ 2017-11-24 13:37       ` Michal Hocko
  0 siblings, 0 replies; 12+ messages in thread
From: Michal Hocko @ 2017-11-24 13:37 UTC (permalink / raw)
  To: Tetsuo Handa; +Cc: linux-mm, akpm, viro, glauber

On Fri 24-11-17 22:26:03, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > As already pointed out, I do not think this is worth it. This function
> > is no different than many others which need error handling. The system
> > will work suboptimally when the shrinker is missing, no question about
> > that, but there is no immediate blow up otherwise. It is not all that
> > hard to find all those places and fix them up. We do not have hundreds
> > of them...
> 
> The system might blow up after two years of uptime. For enterprise systems
> which try to avoid reboots as much as possible, it is important to fix
> known bugs before deploying.

And how exactly __must_check help here? Seriously, if you really _care_
then make sure to fix those rather than add dubious code and keep
arguing over it.
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/2] mm,vmscan: Make unregister_shrinker() no-op if register_shrinker() failed.
  2017-11-24 13:28     ` Michal Hocko
@ 2017-11-25  1:40       ` Tetsuo Handa
  2017-11-27  8:29         ` Michal Hocko
  0 siblings, 1 reply; 12+ messages in thread
From: Tetsuo Handa @ 2017-11-25  1:40 UTC (permalink / raw)
  To: mhocko; +Cc: linux-mm, akpm, viro, glauber, syzkaller

Michal Hocko wrote:
> On Fri 24-11-17 22:21:55, Tetsuo Handa wrote:
> > Michal Hocko wrote:
> > > > Since we can encourage register_shrinker() callers to check for failure
> > > > by marking register_shrinker() as __must_check, unregister_shrinker()
> > > > can stay silent.
> > > 
> > > I am not sure __must_check is the right way. We already do get
> > > allocation warning if the registration fails so silent unregister is
> > > acceptable. Unchecked register_shrinker is a bug like any other
> > > unchecked error path.
> > 
> > I consider that __must_check is the simplest way to find all of
> > unchecked register_shrinker bugs. Why not to encourage users to fix?
> 
> because git grep doesn't require to patch the kernel and still provide
> the information you want.

I can't interpret this line. How git grep relevant?

If all register_shrinker() users were careful enough to check for git history
everytime, we would not have come to current code. It is duty of patch author
to take necessary precautions (for in-tree code) when some API starts to
return an error which previously did not return an error. In this case, it is
duty of author of commit 1d3d4437eae1bb29 ("vmscan: per-node deferred work").

>                           I would understand __must_check if we had
> hundreds users of this api and they come and go quickly.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/2] mm,vmscan: Make unregister_shrinker() no-op if register_shrinker() failed.
  2017-11-25  1:40       ` Tetsuo Handa
@ 2017-11-27  8:29         ` Michal Hocko
  2017-11-29 13:44           ` Tetsuo Handa
  0 siblings, 1 reply; 12+ messages in thread
From: Michal Hocko @ 2017-11-27  8:29 UTC (permalink / raw)
  To: Tetsuo Handa; +Cc: linux-mm, akpm, viro, glauber, syzkaller

On Sat 25-11-17 10:40:13, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > On Fri 24-11-17 22:21:55, Tetsuo Handa wrote:
> > > Michal Hocko wrote:
> > > > > Since we can encourage register_shrinker() callers to check for failure
> > > > > by marking register_shrinker() as __must_check, unregister_shrinker()
> > > > > can stay silent.
> > > > 
> > > > I am not sure __must_check is the right way. We already do get
> > > > allocation warning if the registration fails so silent unregister is
> > > > acceptable. Unchecked register_shrinker is a bug like any other
> > > > unchecked error path.
> > > 
> > > I consider that __must_check is the simplest way to find all of
> > > unchecked register_shrinker bugs. Why not to encourage users to fix?
> > 
> > because git grep doesn't require to patch the kernel and still provide
> > the information you want.
> 
> I can't interpret this line. How git grep relevant?

you do not have to compile to see who is checking the return value.
Seriously there is no need to overcomplicate this. Newly added shrinkers
know the function returns might fail so we just have to handle existing
users and there are not all that many of those.

> If all register_shrinker() users were careful enough to check for git history
> everytime, we would not have come to current code. It is duty of patch author
> to take necessary precautions (for in-tree code) when some API starts to
> return an error which previously did not return an error. In this case, it is
> duty of author of commit 1d3d4437eae1bb29 ("vmscan: per-node deferred work").

Yes I agree, the change was pushed without a due review and necessary
changes.

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/2] mm,vmscan: Make unregister_shrinker() no-op if register_shrinker() failed.
  2017-11-27  8:29         ` Michal Hocko
@ 2017-11-29 13:44           ` Tetsuo Handa
  2017-11-29 13:55             ` Michal Hocko
  0 siblings, 1 reply; 12+ messages in thread
From: Tetsuo Handa @ 2017-11-29 13:44 UTC (permalink / raw)
  To: mhocko; +Cc: linux-mm, akpm, viro, glauber, syzkaller

Michal Hocko wrote:
> On Sat 25-11-17 10:40:13, Tetsuo Handa wrote:
> > Michal Hocko wrote:
> > > On Fri 24-11-17 22:21:55, Tetsuo Handa wrote:
> > > > Michal Hocko wrote:
> > > > > > Since we can encourage register_shrinker() callers to check for failure
> > > > > > by marking register_shrinker() as __must_check, unregister_shrinker()
> > > > > > can stay silent.
> > > > > 
> > > > > I am not sure __must_check is the right way. We already do get
> > > > > allocation warning if the registration fails so silent unregister is
> > > > > acceptable. Unchecked register_shrinker is a bug like any other
> > > > > unchecked error path.
> > > > 
> > > > I consider that __must_check is the simplest way to find all of
> > > > unchecked register_shrinker bugs. Why not to encourage users to fix?
> > > 
> > > because git grep doesn't require to patch the kernel and still provide
> > > the information you want.
> > 
> > I can't interpret this line. How git grep relevant?
> 
> you do not have to compile to see who is checking the return value.
> Seriously there is no need to overcomplicate this. Newly added shrinkers
> know the function returns might fail so we just have to handle existing
> users and there are not all that many of those.

Newly added shrinker users are not always careful. See commit f2517eb76f1f2f7f
("android: binder: Add global lru shrinker to binder") for example.
Unless we send __must_check change to linux.git, people won't notice it.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/2] mm,vmscan: Make unregister_shrinker() no-op if register_shrinker() failed.
  2017-11-29 13:44           ` Tetsuo Handa
@ 2017-11-29 13:55             ` Michal Hocko
  0 siblings, 0 replies; 12+ messages in thread
From: Michal Hocko @ 2017-11-29 13:55 UTC (permalink / raw)
  To: Tetsuo Handa; +Cc: linux-mm, akpm, viro, glauber, syzkaller

On Wed 29-11-17 22:44:45, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > On Sat 25-11-17 10:40:13, Tetsuo Handa wrote:
> > > Michal Hocko wrote:
> > > > On Fri 24-11-17 22:21:55, Tetsuo Handa wrote:
> > > > > Michal Hocko wrote:
> > > > > > > Since we can encourage register_shrinker() callers to check for failure
> > > > > > > by marking register_shrinker() as __must_check, unregister_shrinker()
> > > > > > > can stay silent.
> > > > > > 
> > > > > > I am not sure __must_check is the right way. We already do get
> > > > > > allocation warning if the registration fails so silent unregister is
> > > > > > acceptable. Unchecked register_shrinker is a bug like any other
> > > > > > unchecked error path.
> > > > > 
> > > > > I consider that __must_check is the simplest way to find all of
> > > > > unchecked register_shrinker bugs. Why not to encourage users to fix?
> > > > 
> > > > because git grep doesn't require to patch the kernel and still provide
> > > > the information you want.
> > > 
> > > I can't interpret this line. How git grep relevant?
> > 
> > you do not have to compile to see who is checking the return value.
> > Seriously there is no need to overcomplicate this. Newly added shrinkers
> > know the function returns might fail so we just have to handle existing
> > users and there are not all that many of those.
> 
> Newly added shrinker users are not always careful. See commit f2517eb76f1f2f7f
> ("android: binder: Add global lru shrinker to binder") for example.
> Unless we send __must_check change to linux.git, people won't notice it.

Crap code doesn't really warrant special handling. This is a failure of
reviewers...

Really, if we start abusing __must_check for non-fatal paths then it
will turn into an ignored class of warnings or find workarounds to
silent false positives.

Look, I will not lose more time discussing this borderline thing with
you even though obviously consider this the number one problem. But you
should really think more from a wider perspective rather than
obsessively focus into a unlikely corner case and beat the soul out of
it.

I really do appreciate you started fixing the remaining places of
course, that is the usual way to deal with these issues. But placing
__must_check around is just pointless. If Andrew thinks this is really
worth it, I will not object...
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2017-11-29 13:55 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-24 11:36 [PATCH v2 1/2] mm,vmscan: Make unregister_shrinker() no-op if register_shrinker() failed Tetsuo Handa
2017-11-24 11:36 ` [PATCH v2 2/2] mm,vmscan: Mark register_shrinker() as __must_check Tetsuo Handa
2017-11-24 12:24   ` Michal Hocko
2017-11-24 13:26     ` Tetsuo Handa
2017-11-24 13:37       ` Michal Hocko
2017-11-24 12:21 ` [PATCH v2 1/2] mm,vmscan: Make unregister_shrinker() no-op if register_shrinker() failed Michal Hocko
2017-11-24 13:21   ` Tetsuo Handa
2017-11-24 13:28     ` Michal Hocko
2017-11-25  1:40       ` Tetsuo Handa
2017-11-27  8:29         ` Michal Hocko
2017-11-29 13:44           ` Tetsuo Handa
2017-11-29 13:55             ` 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.