Re: [PATCH v4 2/4] pidfd: add PIDFD_SELF_* sentinels to refer to own thread/process

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Tue, Oct 22, 2024 at 05:53:00PM -0700, Shakeel Butt wrote:
> On Thu, Oct 17, 2024 at 10:05:50PM GMT, Lorenzo Stoakes wrote:
> > It is useful to be able to utilise the pidfd mechanism to reference the
> > current thread or process (from a userland point of view - thread group
> > leader from the kernel's point of view).
> >
> > Therefore introduce PIDFD_SELF_THREAD to refer to the current thread, and
> > PIDFD_SELF_THREAD_GROUP to refer to the current thread group leader.
> >
> > For convenience and to avoid confusion from userland's perspective we alias
> > these:
> >
> > * PIDFD_SELF is an alias for PIDFD_SELF_THREAD - This is nearly always what
> >   the user will want to use, as they would find it surprising if for
> >   instance fd's were unshared()'d and they wanted to invoke pidfd_getfd()
> >   and that failed.
> >
> > * PIDFD_SELF_PROCESS is an alias for PIDFD_SELF_THREAD_GROUP - Most users
> >   have no concept of thread groups or what a thread group leader is, and
> >   from userland's perspective and nomenclature this is what userland
> >   considers to be a process.
>
> Should users use PIDFD_SELF_PROCESS in process_madvise() for self
> madvise() (once the support is added)?

You can use either it will make no difference as both will get you to
current->mm which has to be shared. So I'd go with PIDFD_SELF for brevity
:)

This series and the prerequisites I already added to process_madvise()
already provide support so with this in you can just use this for that.

>
> >
> [...]
> >
> > +static struct pid *pidfd_get_pid_self(unsigned int pidfd, unsigned int *flags)
> > +{
> > +	bool is_thread = pidfd == PIDFD_SELF_THREAD;
> > +	enum pid_type type = is_thread ? PIDTYPE_PID : PIDTYPE_TGID;
> > +	struct pid *pid = *task_pid_ptr(current, type);
> > +
> > +	/* The caller expects an elevated reference count. */
> > +	get_pid(pid);
>
> Do you want this helper to work for scenarios where pid is used across
> context? Otherwise can't we get rid of this get and later put for self?

Yeah I hate doing this but it's to keep things as generic as possible and
to ensure that no user of this logic accidentally drops the reference count
unintentionally + to reduce churn.

I think doing it this way is better than trying to special case the put,
and in any case if you did the 'old' way of referencing yourself you'd have
ended up doing this anyway so it's at least no delta!

>
> > +	return pid;
> > +}
> > +
>
> Overall looks good to me.
>
> Reviewed-by: Shakeel Butt <shakeel.butt@linux.dev>

Thanks!