Libguestfs - Aug 2019 - Re: [nbdkit PATCH 8/8] rate: Atomically set CLOEXEC on fds

On 7/31/19 4:31 PM, Eric Blake wrote:
> The rate filter is potentially opening fds in one thread while another
> thread is processing a fork() in the plugin.  Although the file is not
> open for long, we MUST atomically use CLOEXEC to avoid fd leaks.  This
> one is a bit harder to observe using only the sh plugin, because the
> window is small; you'll have better success at catching the leak by
> using gdb or recompiling code to insert strategic sleeps.
In fact, I have to tweak this commit message: you CAN'T observe this one
with the sh plugin unless you recompile it to use #define THREAD_MODEL
NBDKIT_THREAD_MODEL_SERIALIZE_REQUESTS, as well as introducing the
timing hacks mentioned above (that's because with our current
SERIALIZE_ALL_REQUESTS, there is never more than one thread in
filter/plugin code at a time).

But it does raise an interesting point - if we hit platforms that are
unable to support atomic CLOEXEC, one possibility is a patch that forces
SERIALIZE_ALL_REQUESTS as the maximum parallelism allowed on that
platform (while remaining at our goal of PARALLEL on more competent
systems) - once we do that, the lacking systems will be serialized to
the point that there is no race window where one thread can fork() while
another is obtaining an fd.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Wed, Jul 31, 2019 at 05:01:52PM -0500, Eric Blake
wrote:
> On 7/31/19 4:31 PM, Eric Blake wrote:
> > The rate filter is potentially opening fds in one thread while another
> > thread is processing a fork() in the plugin.  Although the file is not
> > open for long, we MUST atomically use CLOEXEC to avoid fd leaks.  This
> > one is a bit harder to observe using only the sh plugin, because the
> > window is small; you'll have better success at catching the leak
by
> > using gdb or recompiling code to insert strategic sleeps.
> 
> In fact, I have to tweak this commit message: you CAN'T observe this
one
> with the sh plugin unless you recompile it to use #define THREAD_MODEL
> NBDKIT_THREAD_MODEL_SERIALIZE_REQUESTS, as well as introducing the
> timing hacks mentioned above (that's because with our current
> SERIALIZE_ALL_REQUESTS, there is never more than one thread in
> filter/plugin code at a time).
The current nbdkit-sh-plugin is only SERIALIZE_ALL_REQUESTS in order
to make writing the shell scripts a bit more sane.  I believe it could
be fully PARALLEL.

(As an aside: Ideally in future we'll allow the thread model to be
specified by the plugin dynamically.  It's one of the things I thought
I had listed in the TODO file - it wasn't there so I've added it now.)
> But it does raise an interesting point - if we hit platforms that are
> unable to support atomic CLOEXEC, one possibility is a patch that forces
> SERIALIZE_ALL_REQUESTS as the maximum parallelism allowed on that
> platform (while remaining at our goal of PARALLEL on more competent
> systems) - once we do that, the lacking systems will be serialized to
> the point that there is no race window where one thread can fork() while
> another is obtaining an fd.
Yup.  But probably better to encourage those platforms to support
atomic CLOEXEC everywhere.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

On 8/1/19 4:12 AM, Richard W.M. Jones wrote:
> On Wed, Jul 31, 2019 at 05:01:52PM -0500, Eric Blake wrote:
>> On 7/31/19 4:31 PM, Eric Blake wrote:
>>> The rate filter is potentially opening fds in one thread while
another
>>> thread is processing a fork() in the plugin.  Although the file is
not
>>> open for long, we MUST atomically use CLOEXEC to avoid fd leaks. 
This
>>> one is a bit harder to observe using only the sh plugin, because
the
>>> window is small; you'll have better success at catching the
leak by
>>> using gdb or recompiling code to insert strategic sleeps.
>>
>> In fact, I have to tweak this commit message: you CAN'T observe
this one
>> with the sh plugin unless you recompile it to use #define THREAD_MODEL
>> NBDKIT_THREAD_MODEL_SERIALIZE_REQUESTS, as well as introducing the
>> timing hacks mentioned above (that's because with our current
>> SERIALIZE_ALL_REQUESTS, there is never more than one thread in
>> filter/plugin code at a time).
> 
> The current nbdkit-sh-plugin is only SERIALIZE_ALL_REQUESTS in order
> to make writing the shell scripts a bit more sane.  I believe it could
> be fully PARALLEL.
Other than the fact that it uses pipe() instead of pipe2(), I'm not
seeing any other strong reasons why it can't be parallel.  I'll change
patch 9 along those lines.
> 
> (As an aside: Ideally in future we'll allow the thread model to be
> specified by the plugin dynamically.  It's one of the things I thought
> I had listed in the TODO file - it wasn't there so I've added it
now.)
That's because we already have that: See commit afbcd070 and nearby.  So
I'll just revert your TODO change :)
> 
>> But it does raise an interesting point - if we hit platforms that are
>> unable to support atomic CLOEXEC, one possibility is a patch that
forces
>> SERIALIZE_ALL_REQUESTS as the maximum parallelism allowed on that
>> platform (while remaining at our goal of PARALLEL on more competent
>> systems) - once we do that, the lacking systems will be serialized to
>> the point that there is no race window where one thread can fork()
while
>> another is obtaining an fd.
> 
> Yup.  But probably better to encourage those platforms to support
> atomic CLOEXEC everywhere.
Yes, it would be nice for Haiku to realize how much they are losing out
on by not providing it.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org