Libguestfs - Mar 2023 - [libnbd PATCH v3 19/19] socket activation: set LISTEN_FDNAMES

On 3/23/23 20:27, Eric Blake wrote:
> On Thu, Mar 23, 2023 at 01:10:16PM +0100, Laszlo Ersek wrote:
>> When the user calls nbd_set_socket_activation_name before calling
>> nbd_connect_system_socket_activation, pass the name down to the server
>> through LISTEN_FDNAMES.  This has no effect unless the new API has
>> been called to set the socket name to a non-empty string.
>>
>> https://lists.nongnu.org/archive/html/qemu-devel/2023-01/msg06114.html
>>
>> [Original commit message and upstream discussion reference by Rich
Jones;
>> at
>>
<https://listman.redhat.com/archives/libguestfs/2023-January/030558.html>
>> / msgid <20230130225521.1771496-5-rjones at redhat.com>.]
>>
>> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
>> Reviewed-by: Richard W.M. Jones <rjones at redhat.com>
>> ---
>>
>
>> @@ -245,6 +245,9 @@  CONNECT_SA.START:
>>                   "LISTEN_PID=",
"xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx", &pid_ofs);
>>    SACT_VAR_PUSH (sact_var, &num_vars,
>>                   "LISTEN_FDS=", "1", NULL);
>> +  if (h->sact_name != NULL)
>> +    SACT_VAR_PUSH (sact_var, &num_vars,
>> +                   "LISTEN_FDNAMES=", h->sact_name,
NULL);
>>    if (prepare_socket_activation_environment (&env, sact_var,
num_vars) == -1)
>
> If I'm reading this correctly, this does wipe an inherited
> LISTEN_FDNAMES from the environment in the case where the application
> linked with libnbd started life with a (different) socket activation,
> but now the user wants to connect to an nbd server without setting a
> name (default usage, or explicitly requested a name of "").
Good observation; this is a functionality gap that goes back to v1 of
this patch. (I've not investigated the specifics of systemd socket
activation before; but see below.)
> Put another way, SACT_VAR_PUSH as written appears to be only additive
> for replacement purposes (if I pushed a variable, I intend to override
> it in the child process, so don't copy it from environ if one was
> previously there), but not effective for deletion purposes (I don't
> intend to set the variable, but if it is already set in environ, I
> want it omitted in the child's copy).
>
> Is there a way to rework this so that you can pass NULL as the fourth
> parameter as an indication of an unset request (vs. "" when you
want
> it set to the empty string)?  At which point, you would drop the 'if
> (h->sact_name != NULL)', and just blindly use SACT_VAR_PUSH(,
> h->sact_name,).  That has ripple effects earlier in the series to
> support those semantics.
For understanding your point, I have had to read up on systemd socket
activation:

  https://www.freedesktop.org/software/systemd/man/sd_listen_fds.html

Let me quote a part:
> Under specific conditions, the following automatic file descriptor names
are returned:
>
>   [...]
>   "unknown" -- The process received no name for the specific file
>                descriptor from the service manager.
>   [...]
I've also checked the implementation of sd_listen_fds_with_names():

 
https://github.com/systemd/systemd/blob/main/src/libsystemd/sd-daemon/sd-daemon.c

Let me quote the function:
> _public_ int sd_listen_fds_with_names(int unset_environment, char ***names)
{
>         _cleanup_strv_free_ char **l = NULL;
>         bool have_names;
>         int n_names = 0, n_fds;
>         const char *e;
>         int r;
>
>         if (!names)
>                 return sd_listen_fds(unset_environment);
>
>         e = getenv("LISTEN_FDNAMES");
>         if (e) {
>                 n_names = strv_split_full(&l, e, ":",
EXTRACT_DONT_COALESCE_SEPARATORS);
>                 if (n_names < 0) {
>                         unsetenv_all(unset_environment);
>                         return n_names;
>                 }
>
>                 have_names = true;
>         } else
>                 have_names = false;
>
>         n_fds = sd_listen_fds(unset_environment);
>         if (n_fds <= 0)
>                 return n_fds;
>
>         if (have_names) {
>                 if (n_names != n_fds)
>                         return -EINVAL;
>         } else {
>                 r = strv_extend_n(&l, "unknown", n_fds);
>                 if (r < 0)
>                         return r;
>         }
>
>         *names = TAKE_PTR(l);
>
>         return n_fds;
> }
Based on those:

(1) If we pass LISTEN_PID=xxx and LISTEN_FDS=1, and don't pass
    LISTEN_FDNAMES at all, then sd_listen_fds_with_names() will function
    in the nbd server properly, and will return a "names" array with a
    single non-NULL element "unknown", and a terminating null pointer.
    The "unknown" value is specified behavior that socket-activated
    services can rely upon.

(2) If we pass LISTEN_PID=xxx and LISTEN_FDS=1, and just "pass
through"
    an *inherited* LISTEN_FDNAMES variable, then it will (in the general
    case) confuse the nbd server that we start. Namely, if
    LISTEN_FDNAMES has multiple colon-separated elements (more than 1),
    or is the empty string (= 0 elements), then
    sd_listen_fds_with_names() in the nbd server will fail the "n_names
    != n_fds" check, and return (-EINVAL). If LISTEN_FDNAMES happens to
    have one element, then sd_listen_fds_with_names() will succeed, but
    the returned name will confuse the nbd server.

(3) If we pass LISTEN_PID=xxx and LISTEN_FDS=1, and rework the logic in
    this patch to pass LISTEN_FDNAMES="" in case
"h->sact_name" is NULL,
    then sd_listen_fds_with_names() will succeed in the nbd server, but
    the returned single name ("") will most likely confuse it.

(4) If we pass LISTEN_PID=xxx and LISTEN_FDS=1, and rework the logic in
    this patch to pass LISTEN_FDNAMES="unknown" in case
"h->sact_name"
    is NULL, then sd_listen_fds_with_names() will succeed in the nbd
    server, and the single returned name ("unknown") will merge into
    case (1), i.e., as if we had not passed (or had removed)
    LISTEN_FDNAMES in the environment.

Therefore I propose that we implement (4). We're already populating the
LISTEN_* variables ourselves (that is, not relying on systemd library or
daemon logic to fill them in). I see nothing wrong with setting
LISTEN_FDNAMES="unknown" ourselves; again that value is publicly
specified behavior.

Case (4) also appears consistent with repeated calls to
sd_listen_fds_with_names(). If "unset_environment" is set to nonzero
in
one of those calls, then further calls will see the internal
sd_listen_fds() call return 0, and behave as expected. Whereas if
"unset_environment" is never set to zero in those repeated calls, then
"unknown" will continue to be returned from LISTEN_FDNAMES (as if via
case (1)).

Under case (4), we should also update the API documentation in the
previous patch ("generator: Add APIs to get/set the socket activation
socket name"):
> +The parameter C<socket_name> can be a short alphanumeric string.
> +If it is set to the empty string (also the default when the handle
> +is created) then no name is passed to the server.";
we can say there, 'then the name "unknown" will be seen by the
server'.

Thanks for the careful review!
Laszlo

typo:

On 3/24/23 11:32, Laszlo Ersek wrote:
> Case (4) also appears consistent with repeated calls to
> sd_listen_fds_with_names(). If "unset_environment" is set to
nonzero in
> one of those calls, then further calls will see the internal
> sd_listen_fds() call return 0, and behave as expected. Whereas if
> "unset_environment" is never set to zero in those repeated calls,
then
s/zero/nonzero/ on the last line
> "unknown" will continue to be returned from LISTEN_FDNAMES (as if
via
> case (1)).

On 3/24/23 11:32, Laszlo Ersek wrote:
> (2) If we pass LISTEN_PID=xxx and LISTEN_FDS=1, and just "pass
through"
>     an *inherited* LISTEN_FDNAMES variable, then it will (in the general
>     case) confuse the nbd server that we start. Namely, if
>     LISTEN_FDNAMES has multiple colon-separated elements (more than 1),
>     or is the empty string (= 0 elements), then
>     sd_listen_fds_with_names() in the nbd server will fail the
"n_names
>     != n_fds" check, and return (-EINVAL). If LISTEN_FDNAMES happens
to
>     have one element, then sd_listen_fds_with_names() will succeed, but
>     the returned name will confuse the nbd server.
... We might even consider this an existent (current) bug in libnbd, as
we don't do anything about LISTEN_FDNAMES at the moment.

Laszlo

[attempting to loop in systemd folks; this started in libnbd at
https://listman.redhat.com/archives/libguestfs/2023-March/031178.html
- although I may have to retry since I'm not a usual subscriber of
systemd-devel]

On Fri, Mar 24, 2023 at 11:32:26AM +0100, Laszlo Ersek
wrote:
> >> @@ -245,6 +245,9 @@  CONNECT_SA.START:
> >>                   "LISTEN_PID=",
"xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx", &pid_ofs);
> >>    SACT_VAR_PUSH (sact_var, &num_vars,
> >>                   "LISTEN_FDS=", "1", NULL);
> >> +  if (h->sact_name != NULL)
> >> +    SACT_VAR_PUSH (sact_var, &num_vars,
> >> +                   "LISTEN_FDNAMES=", h->sact_name,
NULL);
> >>    if (prepare_socket_activation_environment (&env, sact_var,
num_vars) == -1)
> >
> > If I'm reading this correctly, this does wipe an inherited
> > LISTEN_FDNAMES from the environment in the case where the application
> > linked with libnbd started life with a (different) socket activation,
> > but now the user wants to connect to an nbd server without setting a
> > name (default usage, or explicitly requested a name of "").
> 
> Good observation; this is a functionality gap that goes back to v1 of
> this patch. (I've not investigated the specifics of systemd socket
> activation before; but see below.)
> 
> > Put another way, SACT_VAR_PUSH as written appears to be only additive
> > for replacement purposes (if I pushed a variable, I intend to override
> > it in the child process, so don't copy it from environ if one was
> > previously there), but not effective for deletion purposes (I
don't
> > intend to set the variable, but if it is already set in environ, I
> > want it omitted in the child's copy).
> >
> > Is there a way to rework this so that you can pass NULL as the fourth
> > parameter as an indication of an unset request (vs. "" when
you want
> > it set to the empty string)?  At which point, you would drop the
'if
> > (h->sact_name != NULL)', and just blindly use SACT_VAR_PUSH(,
> > h->sact_name,).  That has ripple effects earlier in the series to
> > support those semantics.
> 
> For understanding your point, I have had to read up on systemd socket
> activation:
> 
>   https://www.freedesktop.org/software/systemd/man/sd_listen_fds.html
> 
> Let me quote a part:
> 
> > Under specific conditions, the following automatic file descriptor
names are returned:
> >
> >   [...]
> >   "unknown" -- The process received no name for the specific
file
> >                descriptor from the service manager.
> >   [...]
> 
> I've also checked the implementation of sd_listen_fds_with_names():
> 
>  
https://github.com/systemd/systemd/blob/main/src/libsystemd/sd-daemon/sd-daemon.c
> 
...
> 
> (2) If we pass LISTEN_PID=xxx and LISTEN_FDS=1, and just "pass
through"
>     an *inherited* LISTEN_FDNAMES variable, then it will (in the general
>     case) confuse the nbd server that we start. Namely, if
>     LISTEN_FDNAMES has multiple colon-separated elements (more than 1),
>     or is the empty string (= 0 elements), then
>     sd_listen_fds_with_names() in the nbd server will fail the
"n_names
>     != n_fds" check, and return (-EINVAL). If LISTEN_FDNAMES happens
to
>     have one element, then sd_listen_fds_with_names() will succeed, but
>     the returned name will confuse the nbd server.
...

Eww - this may be a bigger systematic issue caused by systemd itself,
and we should report it there (done by adding in cc here).  They did
not specify LISTEN_FDNAMES at the time LISTEN_PID was first
documented, so the likelihood of libnbd not being the only application
that happens to leak inherited LISTEN_FDNAMES through to the child
process is non-zero, where this sort of bug will bite more than one
client of systemd socket activation.  And it is this sort of
backwards-incompatibility caused by the systemd extension that they
will need to be more careful of avoiding if they ever add any future
LISTEN_* environment variables.


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

On Fri, Mar 24, 2023 at 11:32:26AM +0100, Laszlo Ersek
wrote:
> 
> (4) If we pass LISTEN_PID=xxx and LISTEN_FDS=1, and rework the logic in
>     this patch to pass LISTEN_FDNAMES="unknown" in case
"h->sact_name"
>     is NULL, then sd_listen_fds_with_names() will succeed in the nbd
>     server, and the single returned name ("unknown") will merge
into
>     case (1), i.e., as if we had not passed (or had removed)
>     LISTEN_FDNAMES in the environment.
> 
> Therefore I propose that we implement (4). We're already populating the
> LISTEN_* variables ourselves (that is, not relying on systemd library or
> daemon logic to fill them in). I see nothing wrong with setting
> LISTEN_FDNAMES="unknown" ourselves; again that value is publicly
> specified behavior.
> 
> Case (4) also appears consistent with repeated calls to
> sd_listen_fds_with_names(). If "unset_environment" is set to
nonzero in
> one of those calls, then further calls will see the internal
> sd_listen_fds() call return 0, and behave as expected. Whereas if
> "unset_environment" is never set to zero in those repeated calls,
then
> "unknown" will continue to be returned from LISTEN_FDNAMES (as if
via
> case (1)).
> 
> Under case (4), we should also update the API documentation in the
> previous patch ("generator: Add APIs to get/set the socket activation
> socket name"):
> 
> > +The parameter C<socket_name> can be a short alphanumeric
string.
> > +If it is set to the empty string (also the default when the handle
> > +is created) then no name is passed to the server.";
> 
> we can say there, 'then the name "unknown" will be seen by
the server'.
Agreed that case (4) (supplying an explicit "unkown") is the best path
forward, and with your analysis that we are not only adding a feature,
but fixing a latent bug which existed ever since systemd added
LISTEN_FDNAMES.

I'll be posting a patch to qemu shortly to unset LISTEN_FDNAMES there
as well.

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