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

[Linus Heckemann]

Christian Schoenebeck <qemu_oss@crudebyte.com> writes:

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

I see, will do!

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

Good call. I'm not familiar with glib, so I didn't know about this :)

>> -            fidp->flags |= FID_NON_RECLAIMABLE;
>
> Why did you remove that? It should still be marked as FID_NON_RECLAIMABLE, no?

Indeed, that was an accident. 

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

You're right. I think the best way around it, though it feels ugly, is
to add a third loop in an "out:".

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

Good point. Will do.


One more thing has occurred to me. I think the reclaiming/reopening
logic will misbehave in the following sequence of events:

1. QEMU reclaims an open fid, losing the file handle
2. The file referred to by the fid is replaced with a different file
   (e.g. via rename or symlink) outside QEMU
3. The file is accessed again by the guest, causing QEMU to reopen a
   _different file_ from before without the guest having performed any
   operations that should cause this to happen.

This is neither introduced nor resolved by my changes. Am I overlooking
something that avoids this (be it documentation that directories exposed
via 9p should not be touched by the host), or is this a real issue? I'm
thinking one could at least detect it by saving inode numbers in
V9fsFidState and comparing them when reopening, but recovering from such
a situation seems difficult.

Linus