From mboxrd@z Thu Jan 1 00:00:00 1970 Date: Wed, 7 Aug 2019 14:17:15 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20190807131715.GE2867@work-vm> References: <20190807093152.10964-1-dgilbert@redhat.com> <20190807124537.GA18557@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190807124537.GA18557@redhat.com> Subject: Re: [Virtio-fs] [PATCH] virtiofsd: Fix lo_destroy crash in g_hash_table_foreach_remove List-Id: Development discussions about virtio-fs List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vivek Goyal Cc: virtio-fs@redhat.com * Vivek Goyal (vgoyal@redhat.com) wrote: > On Wed, Aug 07, 2019 at 10:31:52AM +0100, Dr. David Alan Gilbert (git) wrote: > > From: "Dr. David Alan Gilbert" > > > > 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 > > --- > > 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@redhat.com / Manchester, UK