Re: [Qemu-devel] [PATCH v4 0/7] file descriptor passing using pass-fd

[Kevin Wolf <kwolf@redhat.com>]

Am 05.07.2012 16:22, schrieb Corey Bryant:
>>> For some examples:
>>>
>>> 1. client calls 'add-fd', qemu is now tracking fd=4 with refcount 1, in
>>> use by monitor, as member of fdset1
>>> 2. client crashes, so all tracked fds are visited; fd=4 had not yet been
>>> passed to 'remove-fd', so qemu decrements refcount; refcount of fd=4 is
>>> now 0 so qemu closes it
>>
>> Doesn't the refcount belong to an fdset rather than a single fd? As long
>> as the fdset has still a reference, it's possible to reach the other fds
>> in the set through a reopen. For example:
>>
>> 1. client calls 'add-fd', qemu is now tracking fd=4 (O_RDWR) as a member
>> of fdset1, in use by monitor, refcount 1
>> 2. client calls 'add-fd', qemu is now tracing fd=5 (O_RDONLY) as a
>> member of fdset, in use by monitor, refcount is still 1
>> 3. client calls 'device-add' with /dev/fdset/1 as the backing filename
>> and r/w, so qemu_open() dup()s fd 4 and increments the refcount to 2
>> 4. client crashes, so all tracked fdsets are visited; fdset1 gets its
>> refcount decreased to 1, and both member fds 4 and 5 stay open.
>>
>> If you had refcounted a single fd, fd 5 would be closed now and qemu
>> couldn't switch to O_RDONLY any more.
>>
> 
> If the O_RDONLY is for a future command, it would make more sense to add 
> that fd before that future command.  Or are you passing it at step 2 
> because it is needed for the probe re-open issue?

Come on, requiring realistic examples isn't fair. ;-)

Swap steps 2 and 3 in the example, then step 3 is just the preparation
for a command that uses the new fd. The problem stays the same. Or a
live commit operation could be in flight so that qemu must be able to
switch on completion without libvirt interaction.

There are enough reasons that an fd in an fdset exists and is not
opened, but I can't think of one why it should be dropped.

>>>>> In use by whom? If it's still in use in qemu (as in "in-use flag would
>>>>> be set") and we have a refcount of zero, then that's a bug.
>>>>>
>>>>
>>>> In use by qemu.  I don't think it's a bug.  I think there are situations
>>>> where refcount gets to zero but qemu is still using the fd.
>>>
>>> I think the refcount being non-zero _is_ what defines an fd as being in
>>> use by qemu (including use by the monitor).  Any place you have to close
>>> an fd before reopening it is dangerous; the safe way is always to open
>>> with the new permissions before closing the old permissions.
>>
>> There is one case I'm aware of where we need to be careful: Before
>> opening an image, qemu may probe the format. In this case, the image
>> gets opened twice, and the first close comes before the second open. I'm
>> not entirely sure how hard it would be to get rid of that behaviour.
>>
>> If we can't get rid of it, we have a small window that the refcount
>> doesn't really cover, and if we weren't careful it would become racy.
>> This is why I mentioned earlier that maybe we need to defer the refcount
>> decrease a bit. However, I can't see how the in-use flag would make a
>> difference there. If the refcount can't cover it, the in-use flag can't
>> either.
> 
> Yeah this is a problem.  Could we introduce another flag to cover this?

Adding more refcounts or flags is not a problem, but it doesn't solve it
either. The hard question is when to set that flag.

I believe it may be easier to just change the block layer so that it
opens files only once during bdrv_open().

Kevin