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

[Christian Schoenebeck]

On Montag, 26. September 2022 14:42:06 CEST Linus Heckemann wrote:
> 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.
> ---

Ah, you sent this fix as a separate patch on top. I actually just meant that 
you would take my already queued patch as the latest version (just because I 
had made some minor changes on my end) and adjust that patch further as v4.

Anyway, there are still some things to do here, so maybe you can send your 
patch squashed in the next round ...

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

No worries about reproduction, I will definitely test this thoroughly. ;-)

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

Please use `g_autoptr(GArray)` instead of `GArray *`, that avoids the need for 
explicit calls to g_array_free() below.

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

Why did you remove that? It should still be marked as FID_NON_RECLAIMABLE, no?

> -
> -            /* 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++;

Hmm, bumping the refcount here makes sense, as the 2nd loop may be interrupted 
and the fid otherwise disappear in between, but ...

> +            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;

... this return would then leak all remainder fids that you have bumped the 
refcount for above already.

> +        }
>          put_fid(pdu, fidp);
>      }
> 
> +    g_array_free(to_reopen, TRUE);
> +

With `g_autoptr(GArray)` you can drop both g_array_free() calls.

>      return 0;
>  }

Also: I noticed that your changes in virtfs_reset() would need the same 2-loop 
hack to avoid hash iterator invalidation, as it would also call put_fid() 
inside the loop and be prone for hash iterator invalidation otherwise.

Best regards,
Christian Schoenebeck