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

[NeilBrown]

On Wed, 05 Feb 2014 16:09:51 +0000 David Howells <dhowells at redhat.com> wrote:

> 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?

Yes, that looks just right. Thanks.
I'll let you know when the customer confirms it fixes their problem.

NeilBrown

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 828 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/linux-cachefs/attachments/20140206/98b165b5/attachment.sig>