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

[Alban Crequy]

On Mon, May 18, 2015 at 2:00 PM, Lennart Poettering
<lennart at poettering.net> wrote:
> On Mon, 18.05.15 10:34, Alban Crequy (alban at endocode.com) wrote:
>
>> 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.
>
> Well, we could just invoke it after the barrier stuff has done its
> deed, right before we invoke exec(), no?

Right. Sorry for the noise, I'll send another patch shortly, after I
finish testing it.