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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
