[libvirt] [PATCH v3 3/5] osdep: Enable qemu_open to dup pre-opened fd

[Corey Bryant]

On 06/15/2012 02:42 PM, Eric Blake wrote:
> On 06/15/2012 12:16 PM, Corey Bryant wrote:
>
>>> I think you need to honor flags so that the end use of the fd will be as
>>> if qemu had directly opened the file, rather than just doing a blind dup
>>> with a resulting fd that is in a different state than the caller
>>> expected.  I can think of at least the following cases (there may be
>>> more):
>>
>> I was thinking libvirt would handle all the flag settings on open
>> (obviously since that's how I coded it).  I think you're right with this
>> approach though as QEMU will re-open the same file various times with
>> different flags.
>
> How will libvirt know whether qemu wanted to open a file with O_TRUNCATE
> vs. reusing an entire file, or with O_NONBLOCK or not, and so forth?  I
> think there are just too many qmeu_open() calls with different flags to
> expect libvirt to know how to pre-open all possible fds in such a manner
> that /dev/fd/nnn will be a valid replacement for what qemu would have
> done, in the cases where qemu can instead correct flags itself.
>

I agree completely.

>>
>> There are some flags that I don't think we'll be able to change.  For
>> example: O_RDONLY, O_WRONLY, O_RDWR.  I assume libvirt would open all
>> files O_RDWR.
>
> I agree that this can't be changed, so this is one case where libvirt
> will have to know what the file will used for.  But it is also a case
> where qemu can at least check whether the fd passed in has sufficient
> permissions (fcntl(F_GETFL)) for what qemu wanted to use, and should
> error out here rather than silently succeed here then have a weird EBADF
> failure down the road when the dup'd fd is finally used.  You are right
> that libvirt should always be safe in passing in an O_RDWR fd for
> whatever qemu wants, although that may represent security holes (there's
> reasons for O_RDONLY); and in cases where libvirt is instead passing in
> one end of a pipe, the fd will necessarily be O_RDONLY or O_WRONLY
> (since you can't have an O_RDWR pipe).

Good points.  In addition to flag setting, I'll add some flag checking 
for those flags that can't be set (ie. O_RDONLY, O_WRONLY).

>
>>> Oh, and are we using MSG_CMSG_CLOEXEC where possible (and where not
>>> possible, falling back to fcntl(F_GETFD/F_SETFD) to set FD_CLOEXEC) on
>>> all fds received by 'getfd' and 'pass-fd'?  I can't think of any reason
>>> why 'migrate fd:name' would need to be inheritable, and in the case of
>>> /dev/fd/ parsing, while the dup() result may need to be inheritable, the
>>> original that we are dup'ing from should most certainly be cloexec.
>>
>> It doesn't look like we use MSG_CMSG_CLOEXEC anywhere in QEMU.  I don't
>> think we can modify getfd at this point (compatibility) but we could
>> update pass-fd to set it.  It may make more sense to set it with
>> fcntl(FD_CLOEXEC) in qmp_pass_fd().
>
> That's not atomic.  If we don't care about atomicity (for example, if we
> can guarantee that no other thread in qemu can possibly fork/exec in
> between the monitor thread receiving an fd then converting it to
> cloexec, based on how we hold a mutex), then that's fine.  OR, if we
> make sure _all_ fork() calls sanitize themselves, by closing all fds in
> the getfd/pass-fd list prior to calling exec(), then we don't have to
> even worry about cloexec (but then you have to worry about locking the
> getfd name list, since locking a list to iterate it is not an async-safe
> action and probably can't be done between fork() and exec()).
> Otherwise, using MSG_CMSG_CLOEXEC is the only safe way to avoid leaking
> unintended fds into another thread's child process.

Ok I'm sold.  I'll first look into setting MSG_CMSG_CLOEXEC.

>
>>
>>>
>>> if (flags & O_NONBLOCK)
>>>      use fcntl(F_GETFL/F_SETFL) to set O_NONBLOCK
>>> else
>>>      use fcntl(F_GETFL/F_SETFL) to clear O_NONBLOCK
>>>
>>> or maybe we document that callers of pass-fd must always pass fds with
>>> O_NONBLOCK clear instead of clearing it ourselves.  Or maybe we make
>>> sure part of the process of tying name with fd in the lookup list of
>>> named fds is determining the current O_NONBLOCK state in case future
>>> qemu_open() need it in the opposite state.
>>
>> Just documenting it seems error-prone.  Why not just set/clear it based
>> on the flag passed to qemu_open?
>
> Yep, back to my argument that making libvirt have to know every context
> that qemu will want to open, and what flags it would be using in those
> contexts, is painful compared to having qemu just do the right thing in
> the first place, or gracefully erroring out right away at the point of
> the qemu_open(/dev/fd/nnn).
>

I agree.

-- 
Regards,
Corey