Re: [PATCH 4/4] linux: Add pidfd_getpid

[Luca Boccassi <luca.boccassi@gmail.com>]

On Wed, 19 Apr 2023 at 14:56, Adhemerval Zanella Netto
<adhemerval.zanella@linaro.org> wrote:
> On 19/04/23 07:29, Luca Boccassi wrote:
> >> +parse_fdinfo (const char *l, void *arg)
> >> +{
> >> +  enum { fieldlen = sizeof ("Pid:") - 1 };
> >> +  if (strncmp (l, "Pid:", fieldlen) != 0)
> >> +    return true;
> >> +
> >> +  l += fieldlen;
> >> +
> >> +  char *endp;
> >> +  unsigned long int n = strtoul (l, &endp, 10);
> >> +  if (l == endp || n > INT_MAX)
> >> +    return true;
> >> +
> >> +  *(pid_t *)arg = n;
> >> +  return false;
> >> +}
> >> +
> >> +pid_t
> >> +pidfd_getpid (int fd)
> >> +{
> >> +  if (__glibc_unlikely (fd < 0))
> >> +    {
> >> +      __set_errno (EINVAL);
> >> +      return -1;
> >> +    }
> >> +
> >> +  char fdinfoname[FDINFO_FILENAME_LEN];
> >> +
> >> +  char *p = mempcpy (fdinfoname, FDINFO_TO_FILENAME_PREFIX,
> >> +                 strlen (FDINFO_TO_FILENAME_PREFIX));
> >> +  *_fitoa_word (fd, p, 10, 0) = '\0';
> >> +
> >> +  pid_t pid;
> >> +  if (procutils_read_file (fdinfoname, parse_fdinfo, &pid) == -1)
> >> +    return -1;
> >> +
> >> +  return pid;
> >
> > Having implemented this parsing by hand across 3 projects, it is great
> > to see a glibc helper coming.
> >
> > However, please handle the case of Pid being 0 and -1 explicitly, and
> > return a recognizable errno. fdinfo listing 0 means the pidfd cannot be
> > resolved because it's in a separate pid namespace, so something EREMOTE
> > would suffice. -1 means the process exited, so ESRCH seems like the
> > right error. The distiction between these cases and other errors is
> > important to userspace where we do process tracking, like
> > systemd/dbus/polkit.
>
> Ok, it should a matter to handle the 'Pid:' as signed. However, I am not
> sure if it is really worth to set the errno in cases, since the information
> will already available from the function return itself.

The function returns the pid, so the same information wouldn't be
available. Eg. returning -1 because you can't read proc because of
selinux permissions is different from returning -1 because the process
was reaped and fdinfo contained -1 so it was just passed through. We
need to know this distinction in userspace to handle it properly.
Setting the errno accordingly seems the best way to do that to me.

Kind regards,
Luca Boccassi