[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] libxl: Don't leak self pipes


  • To: Jason Andryuk <jandryuk@xxxxxxxxx>
  • From: Anthony PERARD <anthony.perard@xxxxxxxxxx>
  • Date: Tue, 31 May 2022 18:21:37 +0100
  • Authentication-results: esa4.hc3370-68.iphmx.com; dkim=none (message not signed) header.i=none
  • Cc: <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Juergen Gross <jgross@xxxxxxxx>
  • Delivery-date: Tue, 31 May 2022 17:22:27 +0000
  • Ironport-data: A9a23:k9v3Xq5EzfTBPXnDjPVbCAxRtFDHchMFZxGqfqrLsTDasY5as4F+v moXUTzUbPffNGf3Kd9zaIu+8EgP6pSAyNFrGQJr/no8Hi5G8cbLO4+Ufxz6V8+wwmwvb67FA +E2MISowBUcFyeEzvuVGuG96yE6j8lkf5KkYAL+EnkZqTRMFWFw0HqPp8Zj2tQy2YXiWlvX0 T/Pi5a31GGNimYc3l08s8pvmDs31BglkGpF1rCWTakjUG72zxH5PrpGTU2CByKQrr1vNvy7X 47+IISRpQs1yfuP5uSNyd4XemVSKlLb0JPnZnB+A8BOiTAazsA+PzpS2FPxpi67hh3Q9+2dx umhurT3UFcDB6Pmht8ydB5ZEHpXPrBg2LLudC3XXcy7lyUqclPpyvRqSko3IZcZ6qB8BmQmG f4wcW5XKErZ3qTvnez9GrIEascLdaEHOKsWvG1gyjfIS+4rW5nZT43B5MNC3Sd2jcdLdRrbT 5VEMWo1N0SZC/FJEntJKIogu9mLvXTcLSFqlV2Yq4RuumeGmWSd15CyaYGIK7RmX/59gUKwt m/AuWPjDXkyNtOFziGe2mmxneKJliT+MKoQHbu07O93g3Wcw2USDFsdUl7Tiem0jAuyVsxSL 2QQ+zEytu4i+UqzVN7/Uhak5nmesXYht8F4SrNgrlvXk+yNvljfVjNsoiN9hMIO89ZmbwUk/ EGysPTuBD80mb6Ib1KQ3+LBxd+tAhT5PVPudAddE1ZevYC+/d9j5v7cZo09SfDo17UZDRm1m mnX93Zm2t3/mOZRj82GEUb7byVAT3QjZio8/U3pU22s9WuVj6b1NtXzuTA3ARutRbt1r2VtX 1BewqByFMhUUfmweNWlGY3h5o2B6fefKyH7ilVyBZQn/DnF0yf9INsBu2wgehkyapdsldrVj Kj74Fg52XOuFCHyMf8fj3yZV6zGMpQM5fy6D6uJP7Kik7B6dROd/TEGWKJj9zmFraTYqolmY c3zWZ/1VR4yUP07pBLrFrx1+eJ6mUgDKZb7GMmTI+KPiuLOOhZ4iN4tbTOzUwzOxPrc/V+Iq o0FapfiJtc2eLSWXxQ7OLU7dTgiRUXXz7iv9qS7qsbrztJaJVwc
  • Ironport-hdrordr: A9a23:6sxuJ6kgTpb1UlkeJFLjSOHQXqTpDfIs3DAbv31ZSRFFG/Fxl6 iV8sjz8SWE7Ar5OUtQ/OxoV5PsfZqxz/JICMwqTNCftWrdyQmVxeNZjbcKqgeIc0aVygce79 YCT0EXMqyXMbEQt6fHCWeDfOod/A==
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Tue, May 24, 2022 at 12:31:52PM -0400, Jason Andryuk wrote:
> libxl is leaking self pipes to child processes.  These can be seen when
> running with env var _LIBXL_DEBUG_EXEC_FDS=1:
> 
> libxl: debug: libxl_aoutils.c:593:libxl__async_exec_start: forking to 
> execute: /etc/xen/scripts/vif-bridge online
> [Detaching after fork from child process 5099]
> libxl: execing /etc/xen/scripts/vif-bridge: fd 4 is open to pipe:[46805] with 
> flags 0
> libxl: execing /etc/xen/scripts/vif-bridge: fd 13 is open to pipe:[46807] 
> with flags 0
> libxl: execing /etc/xen/scripts/vif-bridge: fd 14 is open to pipe:[46807] 
> with flags 0
> libxl: execing /etc/xen/scripts/vif-bridge: fd 19 is open to pipe:[48570] 
> with flags 0
> libxl: execing /etc/xen/scripts/vif-bridge: fd 20 is open to pipe:[48570] 
> with flags 0
> 
> (fd 3 is also open, but the check only starts at 4 for some reason.)
> 
> For xl, this is the poller created by libxl_ctx_alloc, the poller
> created by do_domain_create -> libxl__ao_create, and the self pipe for
> libxl__sigchld_needed.  Set CLOEXEC on the FDs so they are not leaked
> into children.
> 
> Signed-off-by: Jason Andryuk <jandryuk@xxxxxxxxx>
> ---
> Maybe the setting wants to move into libxl__pipe_nonblock()?  Poller &
> sigchld are the only callers of that function.

No because we could want a pipe which survive fork/exec. The function
would need to be renamed. I think it's fine to set cloexec on the spot.

> ---
> diff --git a/tools/libs/light/libxl_event.c b/tools/libs/light/libxl_event.c
> index c8bcd13960..8d24613921 100644
> --- a/tools/libs/light/libxl_event.c
> +++ b/tools/libs/light/libxl_event.c
> @@ -1800,6 +1800,9 @@ int libxl__poller_init(libxl__gc *gc, libxl__poller *p)
>      rc = libxl__pipe_nonblock(CTX, p->wakeup_pipe);
>      if (rc) goto out;
>  
> +    libxl_fd_set_cloexec(CTX, p->wakeup_pipe[0], 1);
> +    libxl_fd_set_cloexec(CTX, p->wakeup_pipe[1], 1);

I think that's ok. I tried to find out if pollers needs to survive a
fork/exec, but that doesn't seems to be the case. Pollers are only used
by libxl's event machinery, and in a single CTX I think.

> diff --git a/tools/libs/light/libxl_fork.c b/tools/libs/light/libxl_fork.c
> index 676a14bb28..b13659d231 100644
> --- a/tools/libs/light/libxl_fork.c
> +++ b/tools/libs/light/libxl_fork.c
> @@ -387,6 +387,8 @@ int libxl__sigchld_needed(libxl__gc *gc) /* 
> non-reentrant, idempotent */
>      if (CTX->sigchld_selfpipe[0] < 0) {
>          rc = libxl__pipe_nonblock(CTX, CTX->sigchld_selfpipe);
>          if (rc) goto out;
> +        libxl_fd_set_cloexec(CTX, CTX->sigchld_selfpipe[0], 1);
> +        libxl_fd_set_cloexec(CTX, CTX->sigchld_selfpipe[1], 1);

These ones should be also ok, as the pipe is only used for the SIGCHLD
handler so not shared outside of libxl.


So the patch looks good, and hopefully we don't break libvirt.

Reviewed-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>

Thanks,

-- 
Anthony PERARD



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.