* [PATCH 0/2] mm/shrinker: make unregister_shrinker() less fragile
@ 2015-07-11 2:51 Sergey Senozhatsky
2015-07-11 2:51 ` [PATCH 1/2] mm/shrinker: do not NULL dereference uninitialized shrinker Sergey Senozhatsky
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Sergey Senozhatsky @ 2015-07-11 2:51 UTC (permalink / raw
To: Andrew Morton
Cc: linux-mm, linux-kernel, Sergey Senozhatsky, Sergey Senozhatsky
Hello,
Shrinker API does not handle nicely unregister_shrinker() on a not-registered
->shrinker. Looking at shrinker users, they all have to
(a) carry on some sort of a flag to make sure that "unregister_shrinker()"
will not blow up later
(b) be fishy (potentially can Oops)
(c) access private members `struct shrinker' (e.g. `shrink.list.next')
Change unregister_shrinker() to consider all-zeroes shrinker as
'initialized, but not registered' shrinker, so we can avoid NULL
dereference when unregister_shrinker() accidentally receives such
a struct.
Introduce init_shrinker() function to init `critical' shrinkers members
when the entire shrinker cannot be, for some reason, zeroed out. This
also helps to avoid Oops in unregister_shrinker() in some cases (when
unregister_shrinker() receives not initialized and not registered shrinker).
Sergey Senozhatsky (2):
mm/shrinker: do not NULL dereference uninitialized shrinker
mm/shrinker: add init_shrinker() function
include/linux/shrinker.h | 1 +
mm/vmscan.c | 18 ++++++++++++++++++
2 files changed, 19 insertions(+)
--
2.4.5
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] mm/shrinker: do not NULL dereference uninitialized shrinker
2015-07-11 2:51 [PATCH 0/2] mm/shrinker: make unregister_shrinker() less fragile Sergey Senozhatsky
@ 2015-07-11 2:51 ` Sergey Senozhatsky
2015-07-11 2:51 ` [PATCH 2/2] mm/shrinker: add init_shrinker() function Sergey Senozhatsky
2015-07-11 10:02 ` [PATCH 0/2] mm/shrinker: make unregister_shrinker() less fragile Christoph Hellwig
2 siblings, 0 replies; 10+ messages in thread
From: Sergey Senozhatsky @ 2015-07-11 2:51 UTC (permalink / raw
To: Andrew Morton
Cc: linux-mm, linux-kernel, Sergey Senozhatsky, Sergey Senozhatsky
Consider 'all zeroes' shrinker as 'initialized, but not
registered', and, thus, don't unregister such a shrinker.
This helps to avoid accidental NULL pointer dereferences,
when a zeroed shrinker struct is getting passed to
unregister_shrinker() in error handing path, for example.
Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
mm/vmscan.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/mm/vmscan.c b/mm/vmscan.c
index c8d8282..cadc8a2 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -254,6 +254,12 @@ EXPORT_SYMBOL(register_shrinker);
*/
void unregister_shrinker(struct shrinker *shrinker)
{
+ /*
+ * All-zeroes is 'initialized, but not registered' shrinker.
+ */
+ if (unlikely(!shrinker->list.next))
+ return;
+
down_write(&shrinker_rwsem);
list_del(&shrinker->list);
up_write(&shrinker_rwsem);
--
2.4.5
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] mm/shrinker: add init_shrinker() function
2015-07-11 2:51 [PATCH 0/2] mm/shrinker: make unregister_shrinker() less fragile Sergey Senozhatsky
2015-07-11 2:51 ` [PATCH 1/2] mm/shrinker: do not NULL dereference uninitialized shrinker Sergey Senozhatsky
@ 2015-07-11 2:51 ` Sergey Senozhatsky
2015-07-11 10:02 ` [PATCH 0/2] mm/shrinker: make unregister_shrinker() less fragile Christoph Hellwig
2 siblings, 0 replies; 10+ messages in thread
From: Sergey Senozhatsky @ 2015-07-11 2:51 UTC (permalink / raw
To: Andrew Morton
Cc: linux-mm, linux-kernel, Sergey Senozhatsky, Sergey Senozhatsky
All zeroes shrinker is now treated as 'initialized, but
not registered'. If, for some reason, you can't zero your
shrinker struct (or don't want to) then use init_shrinker()
function. Otherwise, in some cases, unregister_shrinker()
may Oops.
Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
include/linux/shrinker.h | 1 +
mm/vmscan.c | 12 ++++++++++++
2 files changed, 13 insertions(+)
diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h
index 4fcacd9..bffb660 100644
--- a/include/linux/shrinker.h
+++ b/include/linux/shrinker.h
@@ -67,6 +67,7 @@ struct shrinker {
#define SHRINKER_NUMA_AWARE (1 << 0)
#define SHRINKER_MEMCG_AWARE (1 << 1)
+extern void init_shrinker(struct shrinker *);
extern int register_shrinker(struct shrinker *);
extern void unregister_shrinker(struct shrinker *);
#endif
diff --git a/mm/vmscan.c b/mm/vmscan.c
index cadc8a2..4bbcfcf 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -221,6 +221,18 @@ static unsigned long get_lru_size(struct lruvec *lruvec, enum lru_list lru)
}
/*
+ * All-zeroes shrinker considered to be initialized. Use this
+ * function if you can't (don't want to) zero out your shrinker
+ * structure.
+ */
+void init_shrinker(struct shrinker *shrinker)
+{
+ shrinker->nr_deferred = NULL;
+ INIT_LIST_HEAD(&shrinker->list);
+}
+EXPORT_SYMBOL(init_shrinker);
+
+/*
* Add a shrinker callback to be called from the vm.
*/
int register_shrinker(struct shrinker *shrinker)
--
2.4.5
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] mm/shrinker: make unregister_shrinker() less fragile
2015-07-11 2:51 [PATCH 0/2] mm/shrinker: make unregister_shrinker() less fragile Sergey Senozhatsky
2015-07-11 2:51 ` [PATCH 1/2] mm/shrinker: do not NULL dereference uninitialized shrinker Sergey Senozhatsky
2015-07-11 2:51 ` [PATCH 2/2] mm/shrinker: add init_shrinker() function Sergey Senozhatsky
@ 2015-07-11 10:02 ` Christoph Hellwig
2015-07-12 2:47 ` Sergey Senozhatsky
2 siblings, 1 reply; 10+ messages in thread
From: Christoph Hellwig @ 2015-07-11 10:02 UTC (permalink / raw
To: Sergey Senozhatsky
Cc: Andrew Morton, linux-mm, linux-kernel, Sergey Senozhatsky
On Sat, Jul 11, 2015 at 11:51:53AM +0900, Sergey Senozhatsky wrote:
> Hello,
>
> Shrinker API does not handle nicely unregister_shrinker() on a not-registered
> ->shrinker. Looking at shrinker users, they all have to
> (a) carry on some sort of a flag to make sure that "unregister_shrinker()"
> will not blow up later
> (b) be fishy (potentially can Oops)
> (c) access private members `struct shrinker' (e.g. `shrink.list.next')
Ayone who does that is broken. You just need to have clear init (with
proper unwinding) and exit functions and order things properly. It
works like most register/unregister calls and should stay that way.
Maye you you should ty to explain what practical problem you're seeing
to start with.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] mm/shrinker: make unregister_shrinker() less fragile
2015-07-11 10:02 ` [PATCH 0/2] mm/shrinker: make unregister_shrinker() less fragile Christoph Hellwig
@ 2015-07-12 2:47 ` Sergey Senozhatsky
2015-07-13 6:33 ` Christoph Hellwig
0 siblings, 1 reply; 10+ messages in thread
From: Sergey Senozhatsky @ 2015-07-12 2:47 UTC (permalink / raw
To: Christoph Hellwig
Cc: Sergey Senozhatsky, Andrew Morton, linux-mm, linux-kernel,
Sergey Senozhatsky
Hello Christoph,
On (07/11/15 03:02), Christoph Hellwig wrote:
> > Shrinker API does not handle nicely unregister_shrinker() on a not-registered
> > ->shrinker. Looking at shrinker users, they all have to
> > (a) carry on some sort of a flag to make sure that "unregister_shrinker()"
> > will not blow up later
> > (b) be fishy (potentially can Oops)
> > (c) access private members `struct shrinker' (e.g. `shrink.list.next')
>
> Ayone who does that is broken. You just need to have clear init (with
> proper unwinding) and exit functions and order things properly. It
> works like most register/unregister calls and should stay that way.
>
> Maye you you should ty to explain what practical problem you're seeing
> to start with.
Yes, but the main difference here is that it seems that shrinker users
don't tend to treat shrinker registration failures as fatal errors and
just continue with shrinker functionality disabled. And it makes sense.
(copy paste from https://lkml.org/lkml/2015/7/9/751)
> Ayone who does that is broken
I'm afraid, in that case we almost don't have not-broken shrinker users.
-- ignoring register_shrinker() error
: int ldlm_pools_init(void)
: {
: int rc;
:
: rc = ldlm_pools_thread_start();
: if (rc == 0) {
: register_shrinker(&ldlm_pools_srv_shrinker);
: register_shrinker(&ldlm_pools_cli_shrinker);
: }
: return rc;
: }
: EXPORT_SYMBOL(ldlm_pools_init);
:
: void ldlm_pools_fini(void)
: {
: unregister_shrinker(&ldlm_pools_srv_shrinker);
: unregister_shrinker(&ldlm_pools_cli_shrinker);
: ldlm_pools_thread_stop();
: }
: EXPORT_SYMBOL(ldlm_pools_fini);
-- and here
:void i915_gem_shrinker_init(struct drm_i915_private *dev_priv)
:{
: dev_priv->mm.shrinker.scan_objects = i915_gem_shrinker_scan;
: dev_priv->mm.shrinker.count_objects = i915_gem_shrinker_count;
: dev_priv->mm.shrinker.seeks = DEFAULT_SEEKS;
: register_shrinker(&dev_priv->mm.shrinker);
:
: dev_priv->mm.oom_notifier.notifier_call = i915_gem_shrinker_oom;
: register_oom_notifier(&dev_priv->mm.oom_notifier);
:}
-- and here
:int __init gfs2_glock_init(void)
:{
: unsigned i;
...
: register_shrinker(&glock_shrinker);
:
: return 0;
:}
:
:void gfs2_glock_exit(void)
:{
: unregister_shrinker(&glock_shrinker);
: destroy_workqueue(glock_workqueue);
: destroy_workqueue(gfs2_delete_workqueue);
:}
-- and here
:static int __init lowmem_init(void)
:{
: register_shrinker(&lowmem_shrinker);
: return 0;
:}
:
:static void __exit lowmem_exit(void)
:{
: unregister_shrinker(&lowmem_shrinker);
:}
-- accessing private member 'c->shrink.list.next' to distinguish between
'register_shrinker() was successful and need to unregister it' and
'register_shrinker() failed, don't unregister_shrinker() because it
may Oops'
:struct cache_set {
: ...
: struct shrinker shrink;
: ...
:};
:
: ...
:
: void bch_btree_cache_free(struct cache_set *c)
: {
: struct btree *b;
: struct closure cl;
: closure_init_stack(&cl);
:
: if (c->shrink.list.next)
: unregister_shrinker(&c->shrink);
-- and here
:int bch_btree_cache_alloc(struct cache_set *c)
:{
...
: register_shrinker(&c->shrink);
:
:
...
:
:void bch_btree_cache_free(struct cache_set *c)
:{
: struct btree *b;
: struct closure cl;
: closure_init_stack(&cl);
:
: if (c->shrink.list.next)
: unregister_shrinker(&c->shrink);
:
And so on and on.
In fact, 'git grep = register_shrinker' gives only
$ git grep '= register_shrinker'
fs/ext4/extents_status.c: err = register_shrinker(&sbi->s_es_shrinker);
fs/nfsd/nfscache.c: status = register_shrinker(&nfsd_reply_cache_shrinker);
fs/ubifs/super.c: err = register_shrinker(&ubifs_shrinker_info);
mm/huge_memory.c: err = register_shrinker(&huge_zero_page_shrinker);
mm/workingset.c: ret = register_shrinker(&workingset_shadow_shrinker);
The rest is 'broken'.
-ss
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] mm/shrinker: make unregister_shrinker() less fragile
2015-07-12 2:47 ` Sergey Senozhatsky
@ 2015-07-13 6:33 ` Christoph Hellwig
2015-07-13 6:52 ` Sergey Senozhatsky
0 siblings, 1 reply; 10+ messages in thread
From: Christoph Hellwig @ 2015-07-13 6:33 UTC (permalink / raw
To: Sergey Senozhatsky
Cc: Christoph Hellwig, Andrew Morton, linux-mm, linux-kernel,
Sergey Senozhatsky
On Sun, Jul 12, 2015 at 11:47:32AM +0900, Sergey Senozhatsky wrote:
> Yes, but the main difference here is that it seems that shrinker users
> don't tend to treat shrinker registration failures as fatal errors and
> just continue with shrinker functionality disabled. And it makes sense.
>
> (copy paste from https://lkml.org/lkml/2015/7/9/751)
>
I hearily disagree. It's not any less critical than other failures.
The right way forward is to handle register failure properly.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] mm/shrinker: make unregister_shrinker() less fragile
2015-07-13 6:33 ` Christoph Hellwig
@ 2015-07-13 6:52 ` Sergey Senozhatsky
2015-07-13 9:03 ` Christoph Hellwig
0 siblings, 1 reply; 10+ messages in thread
From: Sergey Senozhatsky @ 2015-07-13 6:52 UTC (permalink / raw
To: Christoph Hellwig
Cc: Sergey Senozhatsky, Andrew Morton, linux-mm, linux-kernel,
Sergey Senozhatsky
On (07/12/15 23:33), Christoph Hellwig wrote:
> On Sun, Jul 12, 2015 at 11:47:32AM +0900, Sergey Senozhatsky wrote:
> > Yes, but the main difference here is that it seems that shrinker users
> > don't tend to treat shrinker registration failures as fatal errors and
> > just continue with shrinker functionality disabled. And it makes sense.
> >
> > (copy paste from https://lkml.org/lkml/2015/7/9/751)
> >
>
> I hearily disagree. It's not any less critical than other failures.
Why? In some sense, shrinker callbacks are just a way to be nice.
No one writes a driver just to be able to handle shrinker calls. An
ability to react to those calls is just additional option; it does
not directly affect or limit driver's functionality (at least, it
really should not).
> The right way forward is to handle register failure properly.
In other words, to
(a) keep a flag to signify that register was not successful
or
(b) look at ->shrinker.list.next or ->nr_deferred
or
(c) treat register failures as critical errors. (I sort of
disagree with you here).
-ss
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] mm/shrinker: make unregister_shrinker() less fragile
2015-07-13 6:52 ` Sergey Senozhatsky
@ 2015-07-13 9:03 ` Christoph Hellwig
2015-07-13 9:34 ` Sergey Senozhatsky
0 siblings, 1 reply; 10+ messages in thread
From: Christoph Hellwig @ 2015-07-13 9:03 UTC (permalink / raw
To: Sergey Senozhatsky
Cc: Sergey Senozhatsky, Andrew Morton, linux-mm, linux-kernel
On Mon, Jul 13, 2015 at 03:52:53PM +0900, Sergey Senozhatsky wrote:
> Why? In some sense, shrinker callbacks are just a way to be nice.
> No one writes a driver just to be able to handle shrinker calls. An
> ability to react to those calls is just additional option; it does
> not directly affect or limit driver's functionality (at least, it
> really should not).
No, they are not just nice. They are a fundamental part of memory
management and required to reclaim (often large) amounts of memory.
Nevermind that we don't ignore any other registration time error in
the kernel.
> > The right way forward is to handle register failure properly.
>
> In other words, to
> (a) keep a flag to signify that register was not successful
> or
> (b) look at ->shrinker.list.next or ->nr_deferred
> or
> (c) treat register failures as critical errors. (I sort of
> disagree with you here).
The only important part is here is (c).
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] mm/shrinker: make unregister_shrinker() less fragile
2015-07-13 9:03 ` Christoph Hellwig
@ 2015-07-13 9:34 ` Sergey Senozhatsky
2015-07-14 7:17 ` Christoph Hellwig
0 siblings, 1 reply; 10+ messages in thread
From: Sergey Senozhatsky @ 2015-07-13 9:34 UTC (permalink / raw
To: Christoph Hellwig
Cc: Sergey Senozhatsky, Sergey Senozhatsky, Andrew Morton, linux-mm,
linux-kernel
On (07/13/15 02:03), Christoph Hellwig wrote:
> On Mon, Jul 13, 2015 at 03:52:53PM +0900, Sergey Senozhatsky wrote:
> > Why? In some sense, shrinker callbacks are just a way to be nice.
> > No one writes a driver just to be able to handle shrinker calls. An
> > ability to react to those calls is just additional option; it does
> > not directly affect or limit driver's functionality (at least, it
> > really should not).
>
> No, they are not just nice. They are a fundamental part of memory
> management and required to reclaim (often large) amounts of memory.
Yes. 'Nice' used in a sense that drivers have logic to release the
memory anyway; mm asks volunteers (the drivers that have registered
shrinker callbacks) to release some spare/wasted/etc. when things
are getting tough (the drivers are not aware of that in general).
This is surely important to mm, not to the driver though -- it just
agrees to be 'nice', but even not expected to release any memory at
all (IOW, this is not a contract).
-ss
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] mm/shrinker: make unregister_shrinker() less fragile
2015-07-13 9:34 ` Sergey Senozhatsky
@ 2015-07-14 7:17 ` Christoph Hellwig
0 siblings, 0 replies; 10+ messages in thread
From: Christoph Hellwig @ 2015-07-14 7:17 UTC (permalink / raw
To: Sergey Senozhatsky
Cc: Christoph Hellwig, Sergey Senozhatsky, Andrew Morton, linux-mm,
linux-kernel
On Mon, Jul 13, 2015 at 06:34:42PM +0900, Sergey Senozhatsky wrote:
> Yes. 'Nice' used in a sense that drivers have logic to release the
> memory anyway; mm asks volunteers (the drivers that have registered
> shrinker callbacks) to release some spare/wasted/etc. when things
> are getting tough (the drivers are not aware of that in general).
> This is surely important to mm, not to the driver though -- it just
> agrees to be 'nice', but even not expected to release any memory at
> all (IOW, this is not a contract).
Not registering the shrinker is a plain and simple memory leak. Just
like a missing free your driver will appear to work fine for a while,
but eventually the leaks will bring down the whole system including
your driver.
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2015-07-14 7:18 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-07-11 2:51 [PATCH 0/2] mm/shrinker: make unregister_shrinker() less fragile Sergey Senozhatsky
2015-07-11 2:51 ` [PATCH 1/2] mm/shrinker: do not NULL dereference uninitialized shrinker Sergey Senozhatsky
2015-07-11 2:51 ` [PATCH 2/2] mm/shrinker: add init_shrinker() function Sergey Senozhatsky
2015-07-11 10:02 ` [PATCH 0/2] mm/shrinker: make unregister_shrinker() less fragile Christoph Hellwig
2015-07-12 2:47 ` Sergey Senozhatsky
2015-07-13 6:33 ` Christoph Hellwig
2015-07-13 6:52 ` Sergey Senozhatsky
2015-07-13 9:03 ` Christoph Hellwig
2015-07-13 9:34 ` Sergey Senozhatsky
2015-07-14 7:17 ` Christoph Hellwig
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).