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

[NeilBrown]

On Mon, 3 Feb 2014 11:32:45 +1100 NeilBrown <neilb at suse.de> wrote:

> On Wed, 29 Jan 2014 12:17:15 +0000 David Howells <dhowells at redhat.com> wrote:
> 
> > NeilBrown <neilb at suse.de> wrote:
> > 
> > >  Analysis of the crash dump suggests that fscache_object_destroy, and thus
> > >  __rb_erase_colour, is being called on an object that has already been
> > >  destroy and is no longer in the rb tree.  The rbtree code gets upset and
> > >  crashes.
> > 
> > Not unreasonably...  But which rb_tree?  There are two:
> > 
> >  (1) struct cachefiles_cache::active_nodes.
> > 
> >      This is governed by struct cachefiles_cache::active_lock.
> > 
> >  (2) fscache_object_list.
> > 
> >      This is governed by fscache_object_list_lock.
> > 
> >      Unless you have CONFIG_FSCACHE_OBJECT_LIST=y this isn't present and
> >      fscache_objlist_remove() does nothing - in which case all
> >      fscache_object_destroy() does is release the cookie.
> 
> Ahhh.  Two rb_trees.  Of course.  That does invalidate my theory somewhat.
> 
> 
> > 
> > Can you poke around in the registers, see if any of them point to tree (2)
> > (which is a global variable).
> 
> The only rb_tree that I've looked at the the one for the object being
> destroyed when the machine crashes.
> I walk up to the root and that exactly matches fscache_object_list.rb_node.
> It then walked down to enumerate the whole tree and the object being deleted
> isn't in it.  Nor is the parent of the object being deleted.
> 
> 
> 
> > 
> > >   Thus you can get a race
> > > ...
> > >                                     cachefiles_mark_object_active increments
> > >                                        ->usage (to 1) and drops the lock
> > 
> > This is tree (1).
> > 
> > >      cachefiles_put_object calls
> > >         fscache_object_destroy which
> > >         unlinks from the rb tree.
> > 
> > And this is tree (2).
> > 
> > >   cachefiles_objects live in an rbtree which does not imply a reference to
> > >   the object.
> > 
> > Whilst that is true, they're not allowed to be in the rbtree unless they still
> > have at least one reference outstanding.
> > 
> > Apart from cachefiles_walk_to_object()'s "check_error" labelled part, objects
> > are only rb_erase()'d in cachefiles_drop_object().  This is called from the
> > fscache object state machine (fscache_drop_object) which holds a ref on the
> > cachefiles object until fscache_object_work_func() releases it just prior to
> > returning.
> 
> Thanks - I see that now.  The code does look safe.
> 
> Which makes it all the more surprising that I have this crash-dump :-(
> 
> I also have a crash-dump of a crash in fscache_put_operation() because
> FSCACHE_OP_DEAD is already set.  This is in 3.0 - the code is quite different
> in mainline though none of the changes look like they would fix any
> whatever caused the bug.
> 
> So I have two bug reports which both seem to suggest that something was freed
> twice. Yet I can't see anything that would allow that to happen.
> Most frustrating.
> 
> If you have any ideas, I'd love to hear them :-)

OK, I have a new idea.  I won't try a patch until you confirm I'm probably on
the right track, because there is clearly still a lot for me to learn :-)

I originally said that the symptoms suggested the object was being removed
from the rbtree twice.  However that was not really correct.  The symptoms
actually suggest that the object is being removed 1 more time than it is
added.  I assumed "2 and 1", but now I think "1 and 0".  i.e. it is removed
without ever being added.  The fact that the rbnode looked valid could be a
hangover from when the memory was previously used for a completely different
object.

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?

Thanks,
NeilBrown
-------------- 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/20140205/8cca5f21/attachment.sig>