mm/shrinker: define INIT_SHRINKER macro

RFC In review Sergey Senozhatsky 3 participants 5 messages Jul 10, 2015 – Jul 11, 2015

Subsystems (2)

Memory management Shrinker

lore

Share & tools

lore

Sergey Senozhatsky

author

2015-07-10 01:11 UTC #0 lore raw

RFC 1/1 mm/shrinker: define INIT_SHRINKER macro

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(+)

include/linux/shrinker.h +6 -0

index 4fcacd9..10adfc2 100644
@@ -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)

Signature · 7 lines

2.4.5

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

Andrew Morton

maintainer

2015-07-10 22:32 UTC #1 lore raw

Quoted 10 lines · On Fri, 10 Jul 2015 10:12:11 +0900 Sergey Senozhatsky…

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)

Quoted 7 lines

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

include/linux/shrinker.h struct shrinker { · line 71

Quoted 12 lines

include/linux/shrinker.h +6 -0

@@ -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 [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Sergey Senozhatsky

2015-07-11 01:26 UTC #2 lore raw

Quoted 18 lines · Andrew Morton

Quoted 10 lines
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)
Quoted 7 lines
include/linux/shrinker.h
#define INIT_SHRINKER(s)			\
	do {					\
		(s)->nr_deferred = NULL;	\
		INIT_LIST_HEAD(&(s)->list);	\
	} while (0)

Quoted 3 lines

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.

Quoted 16 lines

include/linux/shrinker.h struct shrinker { · line 71
Quoted 12 lines
include/linux/shrinker.h +6 -0
@@ -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

Quoted 7 lines

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

Andrew Morton

maintainer

2015-07-11 01:33 UTC #3 lore raw

Quoted 11 lines · On Sat, 11 Jul 2015 10:25:13 +0900 Sergey Senozhatsky…

Quoted 11 lines
Quoted 10 lines
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)

Quoted 6 lines

Quoted 3 lines
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.

Quoted 2 lines

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.

Sergey Senozhatsky

2015-07-11 01:49 UTC #4 lore raw

Quoted 21 lines · Andrew Morton

Quoted 11 lines
Quoted 11 lines
Quoted 10 lines
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)
Quoted 6 lines
Quoted 3 lines
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.

Quoted 7 lines

Quoted 2 lines
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