[systemd-devel] [PATCH] nspawn: cloexec extraneous fds

[Alban Crequy]

On Wed, May 13, 2015 at 6:14 PM, Lennart Poettering
<lennart at poettering.net> wrote:
> On Mon, 11.05.15 16:41, Alban Crequy (alban.crequy at gmail.com) wrote:
>
>>  src/nspawn/nspawn.c | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/nspawn/nspawn.c b/src/nspawn/nspawn.c
>> index 71a6239..2e45c3b 100644
>> --- a/src/nspawn/nspawn.c
>> +++ b/src/nspawn/nspawn.c
>> @@ -3739,6 +3739,9 @@ int main(int argc, char *argv[]) {
>>          bool root_device_rw = true, home_device_rw = true, srv_device_rw = true;
>>          _cleanup_close_ int master = -1, image_fd = -1;
>>          _cleanup_fdset_free_ FDSet *fds = NULL;
>> +        _cleanup_fdset_free_ FDSet *misc_fds = NULL;
>> +        int fd;
>> +        Iterator i;
>>          int r, n_fd_passed, loop_nr = -1;
>>          char veth_name[IFNAMSIZ];
>>          bool secondary = false, remove_subvol = false;
>> @@ -3775,7 +3778,11 @@ int main(int argc, char *argv[]) {
>>                          goto finish;
>>                  }
>>          }
>> -        fdset_close_others(fds);
>> +        fdset_new_fill(&misc_fds);
>> +        FDSET_FOREACH(fd, fds, i) {
>> +                fdset_remove(misc_fds, fd);
>> +        }
>> +        fdset_cloexec(misc_fds, true);
>>          log_open();
>
> Do we really need an extra FDSet object for this? Why not just remove
> the fdset_close_others() from the nspawn parent and adding it to the
> child process instead, without depending on O_CLOEXEC? Appears much
> simpler to as it avoids keeping two fdsets around...

fdset_close_others() iterates on the open file descriptor found in
/proc/self/fd/* at the time of the call, so I would need to make sure
it does not harvest the newly opened file descriptors such as the ones
used by the barrier, one end of the kmsg socket pair, one end of the
rtnl socket pair, and possibly others. I would need to use fdset_put()
on each of those fds. Although the fds used by the barrier are
exported in struct Barrier, it is kind of internal structure.

By using fdset_cloexec() instead, I don't need to keep in sync the fds
to add in the FDSet whenever a new fd is added in systemd-nspawn.