[PATCH 1/1] 9pfs: avoid iterator invalidation in v9fs_mark_fids_unreclaim

[Linus Heckemann]

Previously, the yielding in v9fs_reopen_fid and put_fid could result
in other parts of the code modifying the fid table. This would
invalidate the hash table iterator, causing misbehaviour.

Now we ensure that we complete the iteration before yielding, so that
the iterator remains valid throughout the loop, and only reopen the
fids afterwards.
---

@Christian: I still haven't been able to reproduce the issue that this
commit is supposed to fix (I tried building KDE too, no problems), so
it's a bit of a shot in the dark. It certainly still runs and I think it
should fix the issue, but it would be great if you could test it.



 hw/9pfs/9p.c | 46 ++++++++++++++++++++++++++++++----------------
 1 file changed, 30 insertions(+), 16 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index f4c1e37202..825c39e122 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -522,33 +522,47 @@ static int coroutine_fn v9fs_mark_fids_unreclaim(V9fsPDU 
*pdu, V9fsPath *path)
     V9fsFidState *fidp;
     gpointer fid;
     GHashTableIter iter;
+    /*
+     * The most common case is probably that we have exactly one
+     * fid for the given path, so preallocate exactly one.
+     */
+    GArray *to_reopen = g_array_sized_new(FALSE, FALSE, sizeof(V9fsFidState*), 
1);
+    gint i;
 
     g_hash_table_iter_init(&iter, s->fids);
 
+    /*
+     * We iterate over the fid table looking for the entries we need
+     * to reopen, and store them in to_reopen. This is because
+     * reopening them happens asynchronously, allowing the fid table
+     * to be modified in the meantime, invalidating our iterator.
+     */
     while (g_hash_table_iter_next(&iter, &fid, (gpointer *) &fidp)) {
-        /*
-         * Ensure the fid survives a potential clunk request during
-         * v9fs_reopen_fid.
-         */
-        fidp->ref++;
-
         if (fidp->path.size == path->size &&
             !memcmp(fidp->path.data, path->data, path->size)) {
-            /* Mark the fid non reclaimable. */
-            fidp->flags |= FID_NON_RECLAIMABLE;
-
-            /* reopen the file/dir if already closed */
-            err = v9fs_reopen_fid(pdu, fidp);
-            if (err < 0) {
-                put_fid(pdu, fidp);
-                return err;
-            }
+            /*
+             * Ensure the fid survives a potential clunk request during
+             * v9fs_reopen_fid or put_fid.
+             */
+            fidp->ref++;
+            g_array_append_val(to_reopen, fidp);
         }
+    }
 
-        /* We're done with this fid */
+    for (i=0; i < to_reopen->len; i++) {
+        fidp = g_array_index(to_reopen, V9fsFidState*, i);
+        /* reopen the file/dir if already closed */
+        err = v9fs_reopen_fid(pdu, fidp);
+        if (err < 0) {
+            put_fid(pdu, fidp);
+            g_array_free(to_reopen, TRUE);
+            return err;
+        }
         put_fid(pdu, fidp);
     }
 
+    g_array_free(to_reopen, TRUE);
+
     return 0;
 }
 
-- 
2.36.2