[Virtio-fs] [PATCH] virtiofsd: Fix lo_destroy crash in g_hash_table_foreach_remove

[Dr. David Alan Gilbert]

* Vivek Goyal (vgoyal at redhat.com) wrote:
> On Wed, Aug 07, 2019 at 10:31:52AM +0100, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert at redhat.com>
> > 
> > This fixes a crash in lo_destroy since g_hash_table_foreach_remove
> > doesn't like the hashtable changing as it iterates, and unref_inode
> > will remove entries.
> 
> Hi David,
> 
> Got two questions.
> 
> - Shouldn't we take a lo->mutex to make sure parallel hash table
>   modifications are not happening.

See Stefan's big comment (which I just changed the function name in)
which explains that we can't take lo->mutex

> - Also before destroying lo, should we sever connection so that any
>   requests which come after lo_destroy() are not entertained.

No, because in some situations lo_destroy does not happen at the end;
e.g. it happens during a umount and we still have the connection
to be able to remount it.

Dave

> Thanks
> Vivek
> 
> > 
> > Avoid the g_hash_table_foreach_remove and use a dummy iterator to find
> > one element of the table at a time.
> > 
> > Fixes: virtiofsd: fix lo_destroy() resource leaks
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert at redhat.com>
> > ---
> >  contrib/virtiofsd/passthrough_ll.c | 27 +++++++++++++--------------
> >  1 file changed, 13 insertions(+), 14 deletions(-)
> > 
> > diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
> > index cc9c175047..321bbb20be 100644
> > --- a/contrib/virtiofsd/passthrough_ll.c
> > +++ b/contrib/virtiofsd/passthrough_ll.c
> > @@ -2445,18 +2445,6 @@ static void lo_removemapping(fuse_req_t req, struct fuse_session *se,
> >  	fuse_reply_err(req, ret);
> >  }
> >  
> > -static int destroy_inode_cb(gpointer key, gpointer value, gpointer user_data)
> > -{
> > -        struct lo_inode *inode = value;
> > -        struct lo_data *lo = user_data;
> > -
> > -        /* inode->nlookup is normally protected by lo->mutex but see the
> > -         * comment in lo_destroy().
> > -         */
> > -        unref_inode(lo, inode, inode->nlookup);
> > -        return TRUE;
> > -}
> > -
> >  static void lo_destroy(void *userdata, struct fuse_session *se)
> >  {
> >  	struct lo_data *lo = (struct lo_data*) userdata;
> > @@ -2474,10 +2462,21 @@ static void lo_destroy(void *userdata, struct fuse_session *se)
> >          /* Normally lo->mutex must be taken when traversing lo->inodes but
> >           * lo_destroy() is a serialized request so no races are possible here.
> >           *
> > -         * In addition, we cannot acquire lo->mutex since destroy_inode_cb() takes it
> > +         * In addition, we cannot acquire lo->mutex since unref_inode() takes it
> >           * too and this would result in a recursive lock.
> >           */
> > -        g_hash_table_foreach_remove(lo->inodes, destroy_inode_cb, lo);
> > +        while (true) {
> > +                GHashTableIter iter;
> > +                gpointer key, value;
> > +
> > +                g_hash_table_iter_init(&iter, lo->inodes);
> > +                if (!g_hash_table_iter_next(&iter, &key, &value)) {
> > +                        break;
> > +                }
> > +
> > +                struct lo_inode *inode = value;
> > +                unref_inode(lo, inode, inode->nlookup);
> > +        }
> >  }
> >  
> >  static struct fuse_lowlevel_ops lo_oper = {
> > -- 
> > 2.21.0
> > 
--
Dr. David Alan Gilbert / dgilbert at redhat.com / Manchester, UK