[Linux-cachefs] locking/refcount problems in cachefiles.

[David Howells]

NeilBrown <neilb at suse.de> wrote:

> fscache_alloc_object() calls fscache_attach_object() and if that fails it
> calls cache->ops->put_object(), which is cachefiles_put_object() which calls
> fscache_object_destroy(), which calls fscache_objlist_remove() which calls
> rb_erase() to remove the object from fscache_object_list.
> 
> However the object isn't added to fscache_object_list except on the success
> path of fscache_attach_object() where it calls fscache_objlist_add().
> 
> So if fscache_attach_object() fails, then the object wasn't added to the
> objlist rbtree, but is removed from it.  This causes a crash.
> 
> One approach would be to rb_init_node(&object->objlist_link) in
> fscache_object_init, and the test !RB_EMPTY_NODE(&obj->objlist_link) before
> calling rb_erase().
> 
> Does that make sense?  Is the suggested fix best or is there a better way?

Sounds very reasonable.  Something like the attached patch?

David
---
commit fe02fb3ec10932ce07406b1581e28326181fc9d8
Author: David Howells <dhowells at redhat.com>
Date:   Wed Feb 5 15:14:40 2014 +0000

    FS-Cache: Handle removal of unadded object to the fscache_object_list rb tree
    
    When FS-Cache allocates an object, the following sequence of events can occur:
    
     -->fscache_alloc_object()
        -->cachefiles_alloc_object() [via cache->ops->alloc_object]
        <--[returns new object]
        -->fscache_attach_object()
        <--[failed]
        -->cachefiles_put_object() [via cache->ops->put_object]
           -->fscache_object_destroy()
              -->fscache_objlist_remove()
                 -->rb_erase() to remove the object from fscache_object_list.
    
    resulting in a crash in the rbtree code.
    
    The problem is that the object is only added to fscache_object_list on the
    success path of fscache_attach_object() where it calls fscache_objlist_add().
    
    So if fscache_attach_object() fails, the object won't have been added to the
    objlist rbtree.  We do, however, unconditionally try to remove the object from
    the tree.
    
    Thanks to NeilBrown for finding this and suggesting this solution.
    
    Reported-by: NeilBrown <neilb at suse.de>
    Signed-off-by: David Howells <dhowells at redhat.com>

diff --git a/fs/fscache/object-list.c b/fs/fscache/object-list.c
index e1959efad64f..b5ebc2d7d80d 100644
--- a/fs/fscache/object-list.c
+++ b/fs/fscache/object-list.c
@@ -50,6 +50,8 @@ void fscache_objlist_add(struct fscache_object *obj)
 	struct fscache_object *xobj;
 	struct rb_node **p = &fscache_object_list.rb_node, *parent = NULL;
 
+	ASSERT(RB_EMPTY_NODE(&obj->objlist_link));
+
 	write_lock(&fscache_object_list_lock);
 
 	while (*p) {
@@ -75,6 +77,9 @@ void fscache_objlist_add(struct fscache_object *obj)
  */
 void fscache_objlist_remove(struct fscache_object *obj)
 {
+	if (RB_EMPTY_NODE(&obj->objlist_link))
+		return;
+
 	write_lock(&fscache_object_list_lock);
 
 	BUG_ON(RB_EMPTY_ROOT(&fscache_object_list));
diff --git a/fs/fscache/object.c b/fs/fscache/object.c
index 53d35c504240..d3b4539f1651 100644
--- a/fs/fscache/object.c
+++ b/fs/fscache/object.c
@@ -314,6 +314,9 @@ void fscache_object_init(struct fscache_object *object,
 	object->cache = cache;
 	object->cookie = cookie;
 	object->parent = NULL;
+#ifdef CONFIG_FSCACHE_OBJECT_LIST
+	RB_CLEAR_NODE(&object->objlist_link);
+#endif
 
 	object->oob_event_mask = 0;
 	for (t = object->oob_table; t->events; t++)