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

[Lennart Poettering]

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?

Lennart

-- 
Lennart Poettering, Red Hat