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

[Corey Bryant <coreyb@linux.vnet.ibm.com>]

On 07/02/2012 06:31 PM, Eric Blake wrote:
> On 07/02/2012 04:02 PM, Corey Bryant wrote:
>
>> Here's another option that Kevin and I discussed today on IRC.  I've
>> modified a few minor details since the discussion. And Kevin please
>> correct me if anything is wrong.
>>
>> Proposal Four: Pass a set of fds via 'pass-fds'.  The group of fds
>> should all refer to the same file, but may have different access flags
>> (ie. O_RDWR, O_RDONLY).  qemu_open can then dup the fd that has the
>> matching access mode flags.
>
> But this means that libvirt has to open a file O_RDWR up front for any
> file that it _might_ need qemu to reopen later, and that qemu is now
> hanging on to 2 fds per fdset instead of 1 fd for the life of any client
> of the fdset.
>
> I see no reason why libvirt can't pass in an O_RDWR fd when qemu only
> needs to use an O_RDONLY fd; making qemu insist on an O_RDONLY fd makes
> life harder for libvirt.  On the other hand, I can see from a safety
> standpoint that passing in an O_RDWR fd opens up more potential for
> errors than passing in an O_RDONLY fd, but if you don't know up front
> whether you will ever need to write into a file, then it would be better
> to pass in O_RDONLY.  At least I don't see it as a security risk:
> passing in O_RDWR but setting SELinux permissions on the fd to only
> allow read() but not write() via the labeling of the fd may be possible,
> so that libvirt could still prevent accidental writes into an O_RDWR
> file that starts life only needing to service reads.
>
>> pass-fds:
>>      { 'command': 'pass-fds',
>>        'data': {'fdsetname': 'str', '*close': 'bool'},
>>        'returns': 'string' }
>
> This still doesn't solve Dan's complaint that things are not atomic; if
> the monitor dies between the pass-fds and the later use of the fdset, we
> would need a counterpart monitor command to query what fdsets are
> currently known by qemu.  And like you pointed out, you didn't make it
> clear how a timeout mechanism would be implemented to auto-close fds
> that are not consumed in a fixed amount of time - would that be another
> optional parameter to 'pass-fds'?

Yes see the description of the close bool on pass-fds below which 
determines how the fds are closed.

It's not an atomic operation, but it does handle Dan's concern of 
preventing fd leaks.

If libvirt did crash after the pass-fd, then couldn't it just abandon 
the fd set (which will get closed) and libvirt could send a new fd set 
and work with that?

>
> Or do we need a way to initially create a set with only one O_RDONLY fd,
> but later pass in a new O_RDWR fd but associate it with the existing set
> rather than creating a new set (akin to the 'force' option of 'pass-fd'
> in proposal two)?

Yeah I see what you're saying.

I was also wondering if it would make sense for qemu to consume the 
passed fds and use those (ie. no dup()).  This would prevent the issues 
of having to close the fds that linger on the monitor.  But I don't know 
if it's realistic for libvirt to know how many open calls qemu will need 
to make (this would need 1 fd passed per open call).

>
>> close-fds:
>>      { 'command': 'close-fds',
>>        'data': {'fdsetname': 'str' }
>> where:
>>       @fdsetname - the file descriptor set name
>>       @close - *if true QEMU closes the monitor fds when they expire.
>>                if false, fds remain on the monitor until close-fds
>>                command.
>> PRO: Supports reopen
>> PRO: All other commands work without impact by using qemu_open()
>> PRO: No fd leakage if close==true specified
>> CON: Determining when to close fds when close==true may be tricky
>> USAGE:
>> 1. pass-fd FDSET={M} -> returns a string "/dev/fdset/1") that refers to
>> the passed set of fds.
>> 2. drive_add file=/dev/fdset/1 -- qemu_open() uses the first fd from the
>> list that has access flags matching the qemu_open() action flags.
>> 3. block-commit -- qemu_open() reopens "/dev/fdset/1" by using the first
>> fd from the list that has access flags matching the qemu_open() action
>> flags.
>> 4. The monitor fds are closed:
>>     A) *If @close == true, qemu closes the set of fds when the timer
>>        expires
>>     B) If @close == false, libvirt must issue close-fds command to close
>>        the set of fds
>>
>> *How to solve this has yet to be determined.  Kevin mentioned
>> potentially using a bottom-half or a timer in qemu_close().
>
> Still, it is certainly an option worth thinking about, and I'm hoping we
> can come to a solid design that everyone agrees provides the desired
> security and flexibility without having to recode every single existing
> command.
>

I agree.

-- 
Regards,
Corey