qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] systemd: Also clear LISTEN_FDNAMES during systemd socket activation
@ 2023-03-24 15:33 Eric Blake
  2023-03-27  9:15 ` Daniel P. Berrangé
  0 siblings, 1 reply; 3+ messages in thread
From: Eric Blake @ 2023-03-24 15:33 UTC (permalink / raw)
  To: qemu-devel

Some time after systemd documented LISTEN_PID and LISTEN_FDS for
socket activation, they later added LISTEN_FDNAMES; now documented at:
https://www.freedesktop.org/software/systemd/man/sd_listen_fds.html

In particular, look at the implementation of sd_listen_fds_with_names():
https://github.com/systemd/systemd/blob/main/src/libsystemd/sd-daemon/sd-daemon.c

If we ever pass LISTEN_PID=xxx and LISTEN_FDS=n to a child process,
but leave LISTEN_FDNAMES=... unchanged as inherited from our parent
process, then our child process using sd_listen_fds_with_names() might
see a mismatch in the number of names (unexpected -EINVAL failure), or
even if the number of names matches the values of those names may be
unexpected (with even less predictable results).

Usually, this is not an issue - the point of LISTEN_PID is to tell
systemd socket activation to ignore all other LISTEN_* if they were
not directed to this particular pid.  But if we end up consuming a
socket directed to this qemu process, and later decide to spawn a
child process that also needs systemd socket activation, we must
ensure we are not leaking any stale systemd variables through to that
child.  The easiest way to do this is to wipe ALL LISTEN_* variables
at the time we consume a socket, even if we do not yet care about a
LISTEN_FDNAMES passed in from the parent process.

See also https://lists.freedesktop.org/archives/systemd-devel/2023-March/048920.html

Thanks: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 util/systemd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/util/systemd.c b/util/systemd.c
index 5bcac9b4016..ced518f771b 100644
--- a/util/systemd.c
+++ b/util/systemd.c
@@ -51,6 +51,7 @@ unsigned int check_socket_activation(void)
     /* So these are not passed to any child processes we might start. */
     unsetenv("LISTEN_FDS");
     unsetenv("LISTEN_PID");
+    unsetenv("LISTEN_FDNAMES");

     /* So the file descriptors don't leak into child processes. */
     for (i = 0; i < nr_fds; ++i) {
-- 
2.39.2



^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH] systemd: Also clear LISTEN_FDNAMES during systemd socket activation
  2023-03-24 15:33 [PATCH] systemd: Also clear LISTEN_FDNAMES during systemd socket activation Eric Blake
@ 2023-03-27  9:15 ` Daniel P. Berrangé
  2023-04-20 15:05   ` Eric Blake
  0 siblings, 1 reply; 3+ messages in thread
From: Daniel P. Berrangé @ 2023-03-27  9:15 UTC (permalink / raw)
  To: Eric Blake; +Cc: qemu-devel

On Fri, Mar 24, 2023 at 10:33:49AM -0500, Eric Blake wrote:
> Some time after systemd documented LISTEN_PID and LISTEN_FDS for
> socket activation, they later added LISTEN_FDNAMES; now documented at:
> https://www.freedesktop.org/software/systemd/man/sd_listen_fds.html
> 
> In particular, look at the implementation of sd_listen_fds_with_names():
> https://github.com/systemd/systemd/blob/main/src/libsystemd/sd-daemon/sd-daemon.c
> 
> If we ever pass LISTEN_PID=xxx and LISTEN_FDS=n to a child process,
> but leave LISTEN_FDNAMES=... unchanged as inherited from our parent
> process, then our child process using sd_listen_fds_with_names() might
> see a mismatch in the number of names (unexpected -EINVAL failure), or
> even if the number of names matches the values of those names may be
> unexpected (with even less predictable results).
> 
> Usually, this is not an issue - the point of LISTEN_PID is to tell
> systemd socket activation to ignore all other LISTEN_* if they were
> not directed to this particular pid.  But if we end up consuming a
> socket directed to this qemu process, and later decide to spawn a
> child process that also needs systemd socket activation, we must
> ensure we are not leaking any stale systemd variables through to that
> child.  The easiest way to do this is to wipe ALL LISTEN_* variables
> at the time we consume a socket, even if we do not yet care about a
> LISTEN_FDNAMES passed in from the parent process.
> 
> See also https://lists.freedesktop.org/archives/systemd-devel/2023-March/048920.html
> 
> Thanks: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  util/systemd.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|



^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] systemd: Also clear LISTEN_FDNAMES during systemd socket activation
  2023-03-27  9:15 ` Daniel P. Berrangé
@ 2023-04-20 15:05   ` Eric Blake
  0 siblings, 0 replies; 3+ messages in thread
From: Eric Blake @ 2023-04-20 15:05 UTC (permalink / raw)
  To: Daniel P. Berrangé; +Cc: qemu-devel

On Mon, Mar 27, 2023 at 10:15:06AM +0100, Daniel P. Berrangé wrote:
> On Fri, Mar 24, 2023 at 10:33:49AM -0500, Eric Blake wrote:
> > Some time after systemd documented LISTEN_PID and LISTEN_FDS for
> > socket activation, they later added LISTEN_FDNAMES; now documented at:
> > https://www.freedesktop.org/software/systemd/man/sd_listen_fds.html
> > 
> > In particular, look at the implementation of sd_listen_fds_with_names():
> > https://github.com/systemd/systemd/blob/main/src/libsystemd/sd-daemon/sd-daemon.c
> > 
> > If we ever pass LISTEN_PID=xxx and LISTEN_FDS=n to a child process,
> > but leave LISTEN_FDNAMES=... unchanged as inherited from our parent
> > process, then our child process using sd_listen_fds_with_names() might
> > see a mismatch in the number of names (unexpected -EINVAL failure), or
> > even if the number of names matches the values of those names may be
> > unexpected (with even less predictable results).
> > 
> > Usually, this is not an issue - the point of LISTEN_PID is to tell
> > systemd socket activation to ignore all other LISTEN_* if they were
> > not directed to this particular pid.  But if we end up consuming a
> > socket directed to this qemu process, and later decide to spawn a
> > child process that also needs systemd socket activation, we must
> > ensure we are not leaking any stale systemd variables through to that
> > child.  The easiest way to do this is to wipe ALL LISTEN_* variables
> > at the time we consume a socket, even if we do not yet care about a
> > LISTEN_FDNAMES passed in from the parent process.
> > 
> > See also https://lists.freedesktop.org/archives/systemd-devel/2023-March/048920.html
> > 
> > Thanks: Laszlo Ersek <lersek@redhat.com>
> > Signed-off-by: Eric Blake <eblake@redhat.com>
> > ---
> >  util/systemd.c | 1 +
> >  1 file changed, 1 insertion(+)
> 
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

Thanks; queued on my NBD tree for a pull request this week.

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



^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2023-04-20 15:06 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-24 15:33 [PATCH] systemd: Also clear LISTEN_FDNAMES during systemd socket activation Eric Blake
2023-03-27  9:15 ` Daniel P. Berrangé
2023-04-20 15:05   ` Eric Blake

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).