Re: [PATCH] pidfd: add ioctl to retrieve pid info

[Paul Moore]

On Fri, Oct 4, 2024 at 5:29 AM Christian Brauner <brauner@xxxxxxxxxx> wrote:
> On Wed, Oct 02, 2024 at 03:24:33PM GMT, luca.boccassi@xxxxxxxxx wrote:
> > From: Luca Boccassi <bluca@xxxxxxxxxx>
> >
> > A common pattern when using pid fds is having to get information
> > about the process, which currently requires /proc being mounted,
> > resolving the fd to a pid, and then do manual string parsing of
> > /proc/N/status and friends. This needs to be reimplemented over
> > and over in all userspace projects (e.g.: I have reimplemented
> > resolving in systemd, dbus, dbus-daemon, polkit so far), and
> > requires additional care in checking that the fd is still valid
> > after having parsed the data, to avoid races.
> >
> > Having a programmatic API that can be used directly removes all
> > these requirements, including having /proc mounted.

...

> > +             const struct cred *c = get_task_cred(task);
> > +             if (!c)
> > +                     return -ESRCH;
> > +
> > +             info.uid = from_kuid_munged(current_user_ns(), c->uid);
> > +             info.gid = from_kgid_munged(current_user_ns(), c->gid);
> > +     }
> > +
> > +     if (uinfo.request_mask & PIDFD_INFO_CGROUPID) {
> > +             struct cgroup *cgrp = task_css_check(task, pids_cgrp_id, 1)->cgroup;
> > +             if (!cgrp)
> > +                     return -ENODEV;
> > +
> > +             info.cgroupid = cgroup_id(cgrp);
> > +     }
> > +
> > +     if (uinfo.request_mask & PIDFD_INFO_SECURITY_CONTEXT) {
>
> It would make sense for security information to get a separate ioctl so
> that struct pidfd_info just return simple and fast information and the
> security stuff can include things such as seccomp, caps etc pp.

I'm okay with moving the security related info to a separate ioctl,
but I'd like to strongly request that it be merged at the same time as
the process ID related info.  It can be a separate patch as part of a
single patchset if you want to make the ID patch backport friendly for
distros, but I think we should treat the security related info with
the same importance as the ID info.

> > +struct pidfd_info {
> > +        __u64 request_mask;
> > +        __u32 size;
> > +        uint pid;
>
> The size is unnecessary because it is directly encoded into the ioctl
> command.
>
> > +        uint uid;
> > +        uint gid;
> > +        __u64 cgroupid;
> > +        char security_context[NAME_MAX];
> > +} __packed;
>
> The packed attribute should be unnecessary. The structure should simply
> be correctly padded and should use explicitly sized types:
>
> struct pidfd_info {
>         /* Let userspace request expensive stuff explictly. */
>         __u64 request_mask;
>         /* And let the kernel indicate whether it knows about it. */
>         __u64 result_mask;
>         __u32 pid;
>         __u32 uid;
>         __u32 gid;
>         __u64 cgroup_id;
>         __u32 spare0[1];
> };
>
> I'm not sure what LSM info to be put in there and we can just do it as
> an extension.

See my original response to Luca on October 2nd, you were on the To/CC line.

-- 
paul-moore.com