[SRU,T,4/6] fscache: Fix reference overput in fscache_attach_object() error handling

Message ID 20180802041850.22961-5-daniel.axtens@canonical.com
State New
Headers show
  • NFS FSCache Fixes: LP: #1774336, #1776277, #1776254
Related show

Commit Message

Daniel Axtens Aug. 2, 2018, 4:18 a.m.
From: Kiran Kumar Modukuri <kiran.modukuri@gmail.com>

BugLink: https://bugs.launchpad.net/bugs/1776277

When a cookie is allocated that causes fscache_object structs to be
allocated, those objects are initialised with the cookie pointer, but
aren't blessed with a ref on that cookie unless the attachment is
successfully completed in fscache_attach_object().

If attachment fails because the parent object was dying or there was a
collision, fscache_attach_object() returns without incrementing the cookie
counter - but upon failure of this function, the object is released which
then puts the cookie, whether or not a ref was taken on the cookie.

Fix this by taking a ref on the cookie when it is assigned in
fscache_object_init(), even when we're creating a root object.

Analysis from Kiran Kumar:

This bug has been seen in 4.4.0-124-generic #148-Ubuntu kernel

BugLink: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1776277

fscache cookie ref count updated incorrectly during fscache object
allocation resulting in following Oops.

kernel BUG at /build/linux-Y09MKI/linux-4.4.0/fs/fscache/internal.h:321!
kernel BUG at /build/linux-Y09MKI/linux-4.4.0/fs/fscache/cookie.c:639!

Two threads are trying to do operate on a cookie and two objects.

(1) One thread tries to unmount the filesystem and in process goes over a
    huge list of objects marking them dead and deleting the objects.
    cookie->usage is also decremented in following path:

       -> __fscache_relinquish_cookie
        ->BUG_ON(atomic_read(&cookie->usage) <= 0);

(2) A second thread tries to lookup an object for reading data in following

    1) cachefiles_alloc_object
        -> fscache_object_init
           -> assign cookie, but usage not bumped.
    2) fscache_attach_object -> fails in cant_attach_object because the
         cookie's backing object or cookie's->parent object are going away
    3) fscache_put_object
        -> cachefiles_put_object
               ->BUG_ON(atomic_read(&cookie->usage) <= 0);

[NOTE from dhowells] It's unclear as to the circumstances in which (2) can
take place, given that thread (1) is in nfs_kill_super(), however a
conflicting NFS mount with slightly different parameters that creates a
different superblock would do it.  A backtrace from Kiran seems to show
that this is a possibility:

    kernel BUG at/build/linux-Y09MKI/linux-4.4.0/fs/fscache/cookie.c:639!
    RIP: __fscache_cookie_put+0x3a/0x40 [fscache]
    Call Trace:
     __fscache_relinquish_cookie+0x87/0x120 [fscache]
     nfs_fscache_release_super_cookie+0x2d/0xb0 [nfs]
     nfs_kill_super+0x29/0x40 [nfs]

[Fix] Bump up the cookie usage in fscache_object_init, when it is first
being assigned a cookie atomically such that the cookie is added and bumped
up if its refcount is not zero.  Remove the assignment in

I have run ~100 hours of NFS stress tests and not seen this bug recur.

[Regression Potential]
 - Limited to fscache/cachefiles.

Fixes: ccc4fc3d11e9 ("FS-Cache: Implement the cookie management part of the netfs API")
Signed-off-by: Kiran Kumar Modukuri <kiran.modukuri@gmail.com>
Signed-off-by: David Howells <dhowells@redhat.com>
(backported from commit f29507ce66701084c39aeb1b0ae71690cbff3554)
Signed-off-by: Daniel Axtens <daniel.axtens@canonical.com>
 fs/cachefiles/bind.c | 3 ++-
 fs/fscache/cache.c   | 2 +-
 fs/fscache/cookie.c  | 7 ++++---
 fs/fscache/object.c  | 1 +
 4 files changed, 8 insertions(+), 5 deletions(-)


diff --git a/fs/cachefiles/bind.c b/fs/cachefiles/bind.c
index 622f4696e484..2a5b374aff9c 100644
--- a/fs/cachefiles/bind.c
+++ b/fs/cachefiles/bind.c
@@ -219,7 +219,8 @@  static int cachefiles_daemon_add_cache(struct cachefiles_cache *cache)
-	fscache_object_init(&fsdef->fscache, NULL, &cache->cache);
+	fscache_object_init(&fsdef->fscache, &fscache_fsdef_index,
+			    &cache->cache);
 	ret = fscache_add_cache(&cache->cache, &fsdef->fscache, cache->tag);
 	if (ret < 0)
diff --git a/fs/fscache/cache.c b/fs/fscache/cache.c
index f7cff367db7f..5d5c1987b841 100644
--- a/fs/fscache/cache.c
+++ b/fs/fscache/cache.c
@@ -220,6 +220,7 @@  int fscache_add_cache(struct fscache_cache *cache,
 	struct fscache_cache_tag *tag;
+	ASSERTCMP(ifsdef->cookie, ==, &fscache_fsdef_index);
@@ -248,7 +249,6 @@  int fscache_add_cache(struct fscache_cache *cache,
 	if (!cache->kobj)
 		goto error;
-	ifsdef->cookie = &fscache_fsdef_index;
 	ifsdef->cache = cache;
 	cache->fsdef = ifsdef;
diff --git a/fs/fscache/cookie.c b/fs/fscache/cookie.c
index 29d7feb62cf7..269773d7e698 100644
--- a/fs/fscache/cookie.c
+++ b/fs/fscache/cookie.c
@@ -302,6 +302,7 @@  static int fscache_alloc_object(struct fscache_cache *cache,
 		goto error;
+	ASSERTCMP(object->cookie, ==, cookie);
 	object->debug_id = atomic_inc_return(&fscache_object_debug_id);
@@ -356,6 +357,8 @@  static int fscache_attach_object(struct fscache_cookie *cookie,
 	_enter("{%s},{OBJ%x}", cookie->def->name, object->debug_id);
+	ASSERTCMP(object->cookie, ==, cookie);
 	/* there may be multiple initial creations of this object, but we only
@@ -395,9 +398,7 @@  static int fscache_attach_object(struct fscache_cookie *cookie,
-	/* attach to the cookie */
-	object->cookie = cookie;
-	atomic_inc(&cookie->usage);
+	/* Attach to the cookie.  The object already has a ref on it. */
 	hlist_add_head(&object->cookie_link, &cookie->backing_objects);
diff --git a/fs/fscache/object.c b/fs/fscache/object.c
index 53d35c504240..0de8ad801acb 100644
--- a/fs/fscache/object.c
+++ b/fs/fscache/object.c
@@ -313,6 +313,7 @@  void fscache_object_init(struct fscache_object *object,
 	object->store_limit_l = 0;
 	object->cache = cache;
 	object->cookie = cookie;
+	atomic_inc(&cookie->usage);
 	object->parent = NULL;
 	object->oob_event_mask = 0;