[PATCH 0/2] mm/shrinker: make unregister_shrinker() less fragile

[PATCH 0/2] mm/shrinker: make unregister_shrinker() less fragile

[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

--
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>

[PATCH 1/2] mm/shrinker: do not NULL dereference uninitialized shrinker

[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

--
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>

[PATCH 2/2] mm/shrinker: add init_shrinker() function

[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

--
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>

Re: [PATCH 0/2] mm/shrinker: make unregister_shrinker() less fragile

[Christoph Hellwig]

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.

--
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>

Re: [PATCH 0/2] mm/shrinker: make unregister_shrinker() less fragile

[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

--
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>

Re: [PATCH 0/2] mm/shrinker: make unregister_shrinker() less fragile

[Christoph Hellwig]

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.

--
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>

Re: [PATCH 0/2] mm/shrinker: make unregister_shrinker() less fragile

[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

--
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>

Re: [PATCH 0/2] mm/shrinker: make unregister_shrinker() less fragile

[Christoph Hellwig]

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).

--
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>

Re: [PATCH 0/2] mm/shrinker: make unregister_shrinker() less fragile

[Sergey Senozhatsky]

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

--
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>

Re: [PATCH 0/2] mm/shrinker: make unregister_shrinker() less fragile

[Christoph Hellwig]

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.

--
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>