linux-kernel - Re: [PATCH 0/2] mm/shrinker: make unregister

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150712024732.GA787@swordfish>
Date:	Sun, 12 Jul 2015 11:47:32 +0900
From:	Sergey Senozhatsky <sergey.senozhatsky@...il.com>
To:	Christoph Hellwig <hch@...radead.org>
Cc:	Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
	Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org,
	Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
Subject: Re: [PATCH 0/2] mm/shrinker: make unregister_shrinker() less fragile

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