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

[NeilBrown]

On Thu, 6 Feb 2014 11:45:10 +1100 NeilBrown <neilb at suse.de> wrote:

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


I now have definite confirmation that this fixes the customer's problem.
So
  Tested-by: (a customer of) NeilBrown <neilb at suse.de>

Thanks,
NeilBrown


> 
> 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/20140215/491e7f8f/attachment.sig>