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
