[RFC] mm/shrinker: define INIT_SHRINKER macro

[RFC] mm/shrinker: define INIT_SHRINKER macro

[Sergey Senozhatsky]

Hello,

Forked from http://www.gossamer-threads.com/lists/linux/kernel/2209873#2209873
with some adjustments.

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 telling that "unregister_shrinker()" will not blow up... or (b) just
be fishy

: 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);

or (c) access private members `struct shrinker'

: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);


Note that `shrink.list.next' check.


We can't `fix' unregister_shrinker() (by looking at some flag or checking
`!shrinker->nr_deferred'), simply because someone can do something like
this:

:struct foo {
:	const char *b;
: ...
:	struct shrinker s;
:};
:
:void bar(void)
:{
:	struct foo *f = kmalloc(...); /* or kzalloc() to NULL deref it*/
:
:	if (!f)
:		return;
:
:	f->a = kmalloc(...);
:	if (!f->a)
:		goto err;
: ...
:	register_shrinker(...);
: ...
:	return;
:
:err:
:	unregister_shrinker(&f->s);
:			^^^^^^ boom
: ...
:}

Passing a `garbaged' or zeroed out `struct shrinker' to unregister_shrinker()

:void unregister_shrinker(struct shrinker *shrinker)
:{
:        down_write(&shrinker_rwsem);
:        list_del(&shrinker->list);
:        up_write(&shrinker_rwsem);
:        kfree(shrinker->nr_deferred);
:}


I was thinking of a trivial INIT_SHRINKER macro to init `struct shrinker'
internal members (composed in email client, not tested)

include/linux/shrinker.h

#define INIT_SHRINKER(s)			\
	do {					\
		(s)->nr_deferred = NULL;	\
		INIT_LIST_HEAD(&(s)->list);	\
	} while (0)

Of course, every shrinker user need to INIT_SHRINKER() early enough to
guarantee that unregister_shrinker() will be legal should anything go
wrong. Example:

:struct zs_pool *zs_create_pool(char *name, gfp_t flags)
:{
:	..
:+	INIT_SHRINKER(&pool->shrinker);
:
:	pool->name = kstrdup(name, GFP_KERNEL);
:	if (!pool->name)
		goto err;
:	..
:	register_shrinker(&pool->shrinker);
:	..
:	return pool;
:
:err:
:	unregister_shrinker(&pool->shrinker);
:	..
:}

Not much better, but at least some hacks can be avoided and
accidental unregister_shrinker() happening in error path is
safe now.

How does it sound?

---

 include/linux/shrinker.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h
index 4fcacd9..10adfc2 100644
--- a/include/linux/shrinker.h
+++ b/include/linux/shrinker.h
@@ -63,6 +63,12 @@ struct shrinker {
 };
 #define DEFAULT_SEEKS 2 /* A good number if you don't know better. */
 
+#define INIT_SHRINKER(s) 			\
+	do {					\
+		INIT_LIST_HEAD(&(s)->list);	\
+		(s)->nr_deferred = NULL;	\
+	} while (0)
+
 /* Flags */
 #define SHRINKER_NUMA_AWARE	(1 << 0)
 #define SHRINKER_MEMCG_AWARE	(1 << 1)
-- 
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: [RFC] mm/shrinker: define INIT_SHRINKER macro

[Andrew Morton]

On Fri, 10 Jul 2015 10:12:11 +0900 Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> 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 telling that "unregister_shrinker()" will not blow up... or (b) just
> be fishy
> 
> ...
>
> I was thinking of a trivial INIT_SHRINKER macro to init `struct shrinker'
> internal members (composed in email client, not tested)
> 
> include/linux/shrinker.h
> 
> #define INIT_SHRINKER(s)			\
> 	do {					\
> 		(s)->nr_deferred = NULL;	\
> 		INIT_LIST_HEAD(&(s)->list);	\
> 	} while (0)

Spose so.  Although it would be simpler to change unregister_shrinker()
to bale out if list.next==NULL and then say "all zeroes is the
initialized state".

> --- a/include/linux/shrinker.h
> +++ b/include/linux/shrinker.h
> @@ -63,6 +63,12 @@ struct shrinker {
>  };
>  #define DEFAULT_SEEKS 2 /* A good number if you don't know better. */
>  
> +#define INIT_SHRINKER(s) 			\
> +	do {					\
> +		INIT_LIST_HEAD(&(s)->list);	\
> +		(s)->nr_deferred = NULL;	\
> +	} while (0)
> +

The only reason to make this a macro would be so that it can be used at
compile-time, with something like

static struct shrinker my_shrinker = INIT_SHRINKER(&my_shrinker);

But as we're not planning on doing that, we implement it in C, please.

Also, shrinker_init() would be a better name.  Although we already
mucked up shrinker_register() and shrinker_unregister().


--
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: [RFC] mm/shrinker: define INIT_SHRINKER macro

[Sergey Senozhatsky]

On (07/10/15 15:32), Andrew Morton 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 telling that "unregister_shrinker()" will not blow up... or (b) just
> > be fishy
> > 
> > ...
> >
> > I was thinking of a trivial INIT_SHRINKER macro to init `struct shrinker'
> > internal members (composed in email client, not tested)
> > 
> > include/linux/shrinker.h
> > 
> > #define INIT_SHRINKER(s)			\
> > 	do {					\
> > 		(s)->nr_deferred = NULL;	\
> > 		INIT_LIST_HEAD(&(s)->list);	\
> > 	} while (0)
> 
> Spose so.  Although it would be simpler to change unregister_shrinker()
> to bale out if list.next==NULL and then say "all zeroes is the
> initialized state".

Yes, or '->nr_deferred == NULL' -- we can't have NULL ->nr_deferred
in a properly registered shrinker (as of now)

register_shrinker()
...
        shrinker->nr_deferred = kzalloc(size, GFP_KERNEL);
        if (!shrinker->nr_deferred)
                return -ENOMEM;

        down_write(&shrinker_rwsem);
        list_add_tail(&shrinker->list, &shrinker_list);
        up_write(&shrinker_rwsem);
        return 0;
...


But that will not work if someone has accidentally passed not zeroed
out pointer to unregister.

e.g.

...

	struct foo *bar = kmalloc(..) /* no __GFP_ZERO */

	... something goes wrong and we 'goto err' before
	shrinker_register()

err:
	unregister_shrinker(&bar->shrinker);

...


->list.next and ->nr_deferred won't help us here.
That was the reason to have INIT_SHRINKER/shrinker_init().

But adding an additional check to unregister_shrinker() will not harm.


> > --- a/include/linux/shrinker.h
> > +++ b/include/linux/shrinker.h
> > @@ -63,6 +63,12 @@ struct shrinker {
> >  };
> >  #define DEFAULT_SEEKS 2 /* A good number if you don't know better. */
> >  
> > +#define INIT_SHRINKER(s) 			\
> > +	do {					\
> > +		INIT_LIST_HEAD(&(s)->list);	\
> > +		(s)->nr_deferred = NULL;	\
> > +	} while (0)
> > +
> 
> The only reason to make this a macro would be so that it can be used at
> compile-time, with something like
> 
> static struct shrinker my_shrinker = INIT_SHRINKER(&my_shrinker);
> 
> But as we're not planning on doing that, we implement it in C, please.
> 
> Also, shrinker_init() would be a better name.  Although we already
> mucked up shrinker_register() and shrinker_unregister().
> 

Sure. Will do. Thanks.

	-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: [RFC] mm/shrinker: define INIT_SHRINKER macro

[Andrew Morton]

On Sat, 11 Jul 2015 10:25:13 +0900 Sergey Senozhatsky <sergey.senozhatsky@gmail.com> wrote:

> > > I was thinking of a trivial INIT_SHRINKER macro to init `struct shrinker'
> > > internal members (composed in email client, not tested)
> > > 
> > > include/linux/shrinker.h
> > > 
> > > #define INIT_SHRINKER(s)			\
> > > 	do {					\
> > > 		(s)->nr_deferred = NULL;	\
> > > 		INIT_LIST_HEAD(&(s)->list);	\
> > > 	} while (0)
> > 
> > Spose so.  Although it would be simpler to change unregister_shrinker()
> > to bale out if list.next==NULL and then say "all zeroes is the
> > initialized state".
> 
> Yes, or '->nr_deferred == NULL' -- we can't have NULL ->nr_deferred
> in a properly registered shrinker (as of now)

list.next seems safer because that will always be non-zero.  But
whatever - we can change it later.
 
> But that will not work if someone has accidentally passed not zeroed
> out pointer to unregister.

I wouldn't worry about that really.  If you pass a pointer to
uninitialized memory, the kernel will explode.  That's true of just
about every pointer-accepting function in the kernel.


--
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: [RFC] mm/shrinker: define INIT_SHRINKER macro

[Sergey Senozhatsky]

On (07/10/15 18:33), Andrew Morton wrote:
> > > > I was thinking of a trivial INIT_SHRINKER macro to init `struct shrinker'
> > > > internal members (composed in email client, not tested)
> > > > 
> > > > include/linux/shrinker.h
> > > > 
> > > > #define INIT_SHRINKER(s)			\
> > > > 	do {					\
> > > > 		(s)->nr_deferred = NULL;	\
> > > > 		INIT_LIST_HEAD(&(s)->list);	\
> > > > 	} while (0)
> > > 
> > > Spose so.  Although it would be simpler to change unregister_shrinker()
> > > to bale out if list.next==NULL and then say "all zeroes is the
> > > initialized state".
> > 
> > Yes, or '->nr_deferred == NULL' -- we can't have NULL ->nr_deferred
> > in a properly registered shrinker (as of now)
> 
> list.next seems safer because that will always be non-zero.  But
> whatever - we can change it later.
>  
> > But that will not work if someone has accidentally passed not zeroed
> > out pointer to unregister.
> 
> I wouldn't worry about that really.  If you pass a pointer to
> uninitialized memory, the kernel will explode.  That's true of just
> about every pointer-accepting function in the kernel.
>

True. But with shrinker it's hard to say whether we have a properly
initialized shrinker embedded in our `struct foo' or we don't (unless
we treat register_shrinker() errors as a show stopper) by simply looking at
shrinker struct (w/o touching it's private members). In zsmalloc, for
instance, we don't consider failed register_shrinker() to be critical
enough to forbid zs_pool creation and usage. It makes things harder later
in zs_destroy_pool(), because we need to carry some sort of flag for that
purpose. But `list.next' check in unregister_shrinker() would suffice in
zsmalloc case, I must admit, because we kzalloc() the entire zs_pool
struct.

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