[v4,0/4] introduce PIDFD_SELF* sentinels

If you wish to utilise a pidfd interface to refer to the current process or
thread it is rather cumbersome, requiring something like:

	int pidfd = pidfd_open(getpid(), 0 or PIDFD_THREAD);

	...

	close(pidfd);

Or the equivalent call opening /proc/self. It is more convenient to use a
sentinel value to indicate to an interface that accepts a pidfd that we
simply wish to refer to the current process thread.

This series introduces sentinels for this purposes which can be passed as
the pidfd in this instance rather than having to establish a dummy fd for
this purpose.

It is useful to refer to both the current thread from the userland's
perspective for which we use PIDFD_SELF, and the current process from the
userland's perspective, for which we use PIDFD_SELF_PROCESS.

There is unfortunately some confusion between the kernel and userland as to
what constitutes a process - a thread from the userland perspective is a
process in userland, and a userland process is a thread group (more
specifically the thread group leader from the kernel perspective). We
therefore alias things thusly:

* PIDFD_SELF_THREAD aliased by PIDFD_SELF - use PIDTYPE_PID.
* PIDFD_SELF_THREAD_GROUP alised by PIDFD_SELF_PROCESS - use PIDTYPE_TGID.

In all of the kernel code we refer to PIDFD_SELF_THREAD and
PIDFD_SELF_THREAD_GROUP. However we expect users to use PIDFD_SELF and
PIDFD_SELF_PROCESS.

This matters for cases where, for instance, a user unshare()'s FDs or does
thread-specific signal handling and where the user would be hugely confused
if the FDs referenced or signal processed referred to the thread group
leader rather than the individual thread.

We ensure that pidfd_send_signal() and pidfd_getfd() work correctly, and
assert as much in selftests. All other interfaces except setns() will work
implicitly with this new interface, however it doesn't make sense to test
waitid(P_PIDFD, ...) as waiting on ourselves is a blocking operation.

In the case of setns() we explicitly disallow use of PIDFD_SELF* as it
doesn't make sense to obtain the namespaces of our own process, and it
would require work to implement this functionality there that would be of
no use.

We also do not provide the ability to utilise PIDFD_SELF* in ordinary fd
operations such as open() or poll(), as this would require extensive work
and be of no real use.

v4:
* Avoid returning an fd in the __pidfd_get_pid() function as pointed out by
  Christian, instead simply always pin the pid and maintain fd scope in the
  helper alone.
* Add wrapper header file in tools/include/linux to allow for import of
  UAPI pidfd.h header without encountering the collision between system
  fcntl.h and linux/fcntl.h as discussed with Shuah and John.
* Fixup tests to import the UAPI pidfd.h header working around conflicts
  between system fcntl.h and linux/fcntl.h which the UAPI pidfd.h imports,
  as reported by Shuah.
* Use an int for pidfd_is_self_sentinel() to avoid any dependency on
  stdbool.h in userland.

v3:
* Do not fput() an invalid fd as reported by kernel test bot.
* Fix unintended churn from moving variable declaration.
https://lore.kernel.org/linux-mm/cover.1729073310.git.lorenzo.stoakes@oracle.com/

v2:
* Fix tests as reported by Shuah.
* Correct RFC version lore link.
https://lore.kernel.org/linux-mm/cover.1728643714.git.lorenzo.stoakes@oracle.com/

Non-RFC v1:
* Removed RFC tag - there seems to be general consensus that this change is
  a good idea, but perhaps some debate to be had on implementation. It
  seems sensible then to move forward with the RFC flag removed.
* Introduced PIDFD_SELF_THREAD, PIDFD_SELF_THREAD_GROUP and their aliases
  PIDFD_SELF and PIDFD_SELF_PROCESS respectively.
* Updated testing accordingly.
https://lore.kernel.org/linux-mm/cover.1728578231.git.lorenzo.stoakes@oracle.com/

RFC version:
https://lore.kernel.org/linux-mm/cover.1727644404.git.lorenzo.stoakes@oracle.com/


Lorenzo Stoakes (4):
  pidfd: extend pidfd_get_pid() and de-duplicate pid lookup
  pidfd: add PIDFD_SELF_* sentinels to refer to own thread/process
  selftests: pidfd: add pidfd.h UAPI wrapper
  selftests: pidfd: add tests for PIDFD_SELF_*

 include/linux/pid.h                           |  34 ++++-
 include/uapi/linux/pidfd.h                    |  15 ++
 kernel/exit.c                                 |   3 +-
 kernel/nsproxy.c                              |   1 +
 kernel/pid.c                                  |  65 +++++---
 kernel/signal.c                               |  29 +---
 tools/include/linux/pidfd.h                   |  14 ++
 tools/testing/selftests/pidfd/Makefile        |   3 +-
 tools/testing/selftests/pidfd/pidfd.h         |   2 +
 .../selftests/pidfd/pidfd_getfd_test.c        | 141 ++++++++++++++++++
 .../selftests/pidfd/pidfd_setns_test.c        |  11 ++
 tools/testing/selftests/pidfd/pidfd_test.c    |  76 ++++++++--
 12 files changed, 333 insertions(+), 61 deletions(-)
 create mode 100644 tools/include/linux/pidfd.h

On 10/17/24 15:45, John Hubbard wrote:
> On 10/17/24 2:05 PM, Lorenzo Stoakes wrote:
>> Conflicts can arise between system fcntl.h and linux/fcntl.h, imported by
>> the linux/pidfd.h UAPI header.
>>
>> Work around this by adding a wrapper for linux/pidfd.h to
>> tools/include/ which sets the linux/fcntl.h header guard ahead of
>> importing the pidfd.h header file.
>>
>> Adjust the pidfd selftests Makefile to reference this include directory and
>> put it at a higher precidence than any make header installed headers to
>> ensure the wrapper is preferred.
> 
> ...but we are not actually using the installed headers, now. And we intend
> to continue avoiding them. So the ordering shouldn't matter. More below:
> 
>>
>> This way we can directly import the UAPI header file without issue, use the
>> latest system header file without having to duplicate anything.
>>
>> Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>
>> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>> ---
>>   tools/include/linux/pidfd.h            | 14 ++++++++++++++
>>   tools/testing/selftests/pidfd/Makefile |  3 +--
>>   2 files changed, 15 insertions(+), 2 deletions(-)
>>   create mode 100644 tools/include/linux/pidfd.h
>>
>> diff --git a/tools/include/linux/pidfd.h b/tools/include/linux/pidfd.h
>> new file mode 100644
>> index 000000000000..113c8023072d
>> --- /dev/null
>> +++ b/tools/include/linux/pidfd.h
>> @@ -0,0 +1,14 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +
>> +#ifndef _TOOLS_LINUX_PIDFD_H
>> +#define _TOOLS_LINUX_PIDFD_H
>> +
>> +/*
>> + * Some systems have issues with the linux/fcntl.h import in linux/pidfd.h, so
>> + * work around this by setting the header guard.
>> + */
>> +#define _LINUX_FCNTL_H
>> +#include "../../../include/uapi/linux/pidfd.h"
>> +#undef _LINUX_FCNTL_H
> 
> Oh shoot, I think you, Shuah and I were referring to different uapi locations,
> the whole time. And so the basic approach is different after all.
> 
> Your include path above actually refers to:
> 
>      $(top_srcdir)/include/uapi/linux/fcntl.h

Correct. I am glad we are on the same page now.

> 
> ...but what I was intending was to copy a snapshot of that file (or a
> snapshot from the one generated by "make headers"), to here:
> 
>      $(top_srcdir)/tools/include/uapi/linux/fcntl.h

So why do the copy and snapshot. Anytime you build userspace in the
reoo - you will need to run "make headers: whether you install
them under tools/include or include.

  
> 
> ...and then use $(TOOLS_INCLUDES), which is already in selftests/lib.mk,
> for that reason: to be available to all of the kselftests:
> 
>      TOOLS_INCLUDES := -isystem $(top_srcdir)/tools/include/uapi

Yes some tests do include that.

> 
> The reasoning for this directory is further explained here:
> 
>      tools/include/uapi/README
> 
> (And I see that selftests/proc has started using $(TOOLS_INCLUDES), that's
> progress.)

Yes the same problems apply here - what complicates this more is
selftests are supposed to test kernel changes, hence the need to
include latest kernel headers. The simple solution is adding a
dependency so we don't have to duplicate the headers. I don't
believe the perf solution works here. We will have to figure out
a solution.

> 
> And now, it's possible to change fcntl.h in place, instead of using a wrapper.
> Although either way seems OK to me. (I'm sort of ignoring the details of
> the actual header file conflict itself, for now.)
> 
> 
>> +
>> +#endif /* _TOOLS_LINUX_PIDFD_H */
>> diff --git a/tools/testing/selftests/pidfd/Makefile b/tools/testing/selftests/pidfd/Makefile
>> index d731e3e76d5b..f5038c9dae14 100644
>> --- a/tools/testing/selftests/pidfd/Makefile
>> +++ b/tools/testing/selftests/pidfd/Makefile
>> @@ -1,8 +1,7 @@
>>   # SPDX-License-Identifier: GPL-2.0-only
>> -CFLAGS += -g $(KHDR_INCLUDES) -pthread -Wall
>> +CFLAGS += -g -isystem $(top_srcdir)/tools/include $(KHDR_INCLUDES) -pthread -Wall
> 
> Instead, it would look like this, which now mostly matches selftests/mm/Makefile,
> which is also helpful, because eventually this can be factored into a common
> piece for all selftests:
> 
>      CFLAGS += -g -isystem $(KHDR_INCLUDES) $(TOOLS_INCLUDES) -pthread -Wall

KHDR_INCLUDES is "make headers" location under the root directory. So what
happens when you add TOOLS_INCLUDES to it.

Does "make kselftest-all" work as it is supposed to? If it and all tests
can build then I am all for it.

> 
> I apologize for just now noticing this! And these kselftests shouldn't require
> so much fussing around, I know. But once we get this just right, it will work
> well and last a long time. :)
> 


On the contrary if we don't discuss/fuss and get this right, we have to
deal with changes like adding local defines and adhoc approaches in
individual tests - that is one reason we made the "make headers"
as a dependency. I would like to solve the problem of proliferation
of local defines and even system calls in some cases.

For now I am going let this patch go through as it is important to
add tests.

My goals are simple:

- no local defines unless it is abslulutely necessary
- be able to build tests that add coverage for new kernel
   api and features before we release the kernel.
- make it easier for CIs to build and run tests
- continue to have tests works for kernel developres
   e.g: mm developers build tests in mm directory. They
   don't see the issues that crop up in CIs or running
   the entire kselftest default run like CIs do.

Adhoc changes break some use-cases.

thanks,
-- Shuah

On Thu, Oct 17, 2024 at 02:45:43PM -0700, John Hubbard wrote:
> On 10/17/24 2:05 PM, Lorenzo Stoakes wrote:
> > Conflicts can arise between system fcntl.h and linux/fcntl.h, imported by
> > the linux/pidfd.h UAPI header.
> >
> > Work around this by adding a wrapper for linux/pidfd.h to
> > tools/include/ which sets the linux/fcntl.h header guard ahead of
> > importing the pidfd.h header file.
> >
> > Adjust the pidfd selftests Makefile to reference this include directory and
> > put it at a higher precidence than any make header installed headers to
> > ensure the wrapper is preferred.
>
> ...but we are not actually using the installed headers, now. And we intend
> to continue avoiding them. So the ordering shouldn't matter. More below:
>
> >
> > This way we can directly import the UAPI header file without issue, use the
> > latest system header file without having to duplicate anything.
> >
> > Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> > ---
> >   tools/include/linux/pidfd.h            | 14 ++++++++++++++
> >   tools/testing/selftests/pidfd/Makefile |  3 +--
> >   2 files changed, 15 insertions(+), 2 deletions(-)
> >   create mode 100644 tools/include/linux/pidfd.h
> >
> > diff --git a/tools/include/linux/pidfd.h b/tools/include/linux/pidfd.h
> > new file mode 100644
> > index 000000000000..113c8023072d
> > --- /dev/null
> > +++ b/tools/include/linux/pidfd.h
> > @@ -0,0 +1,14 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +
> > +#ifndef _TOOLS_LINUX_PIDFD_H
> > +#define _TOOLS_LINUX_PIDFD_H
> > +
> > +/*
> > + * Some systems have issues with the linux/fcntl.h import in linux/pidfd.h, so
> > + * work around this by setting the header guard.
> > + */
> > +#define _LINUX_FCNTL_H
> > +#include "../../../include/uapi/linux/pidfd.h"
> > +#undef _LINUX_FCNTL_H
>
> Oh shoot, I think you, Shuah and I were referring to different uapi locations,
> the whole time. And so the basic approach is different after all.
>
> Your include path above actually refers to:
>
>     $(top_srcdir)/include/uapi/linux/fcntl.h
>
> ...but what I was intending was to copy a snapshot of that file (or a
> snapshot from the one generated by "make headers"), to here:
>
>     $(top_srcdir)/tools/include/uapi/linux/fcntl.h

Yeah my first version of this used the uapi one but I thought doing that
might conflict with snapshotting? Also it'd mean you'd absolutely have to
have the $(TOOLS_INCLUDES) earlier in the include priority list and better
maybe to special case in this instance.

>
> ...and then use $(TOOLS_INCLUDES), which is already in selftests/lib.mk,
> for that reason: to be available to all of the kselftests:
>
>     TOOLS_INCLUDES := -isystem $(top_srcdir)/tools/include/uapi
>
> The reasoning for this directory is further explained here:
>
>     tools/include/uapi/README
>
> (And I see that selftests/proc has started using $(TOOLS_INCLUDES), that's
> progress.)
>
> And now, it's possible to change fcntl.h in place, instead of using a wrapper.
> Although either way seems OK to me. (I'm sort of ignoring the details of
> the actual header file conflict itself, for now.)

The fcntl.h and linux/fcntl.h conflict is apparently a rather well-known
horror show. It's a difficult one to resolve as the UAPI pidfd.h header
needs O_xxx defines but we also need to include this header in kernel code.

An #ifdef __KERNEL__ block might be a solution here but fixing that is out
of scope for these changes.

>
>
> > +
> > +#endif /* _TOOLS_LINUX_PIDFD_H */
> > diff --git a/tools/testing/selftests/pidfd/Makefile b/tools/testing/selftests/pidfd/Makefile
> > index d731e3e76d5b..f5038c9dae14 100644
> > --- a/tools/testing/selftests/pidfd/Makefile
> > +++ b/tools/testing/selftests/pidfd/Makefile
> > @@ -1,8 +1,7 @@
> >   # SPDX-License-Identifier: GPL-2.0-only
> > -CFLAGS += -g $(KHDR_INCLUDES) -pthread -Wall
> > +CFLAGS += -g -isystem $(top_srcdir)/tools/include $(KHDR_INCLUDES) -pthread -Wall
>
> Instead, it would look like this, which now mostly matches selftests/mm/Makefile,
> which is also helpful, because eventually this can be factored into a common
> piece for all selftests:
>
>     CFLAGS += -g -isystem $(KHDR_INCLUDES) $(TOOLS_INCLUDES) -pthread -Wall
>
> I apologize for just now noticing this! And these kselftests shouldn't require
> so much fussing around, I know. But once we get this just right, it will work
> well and last a long time. :)

Yeah I know, but this won't work due to the header conflict, I was doing
this previously.

Also doing it this way means that uapi snapshot doesn't override the usr/
one if you have that, which I guess you want?

>
> thanks,
> --
> John Hubbard

On 10/17/24 11:49 PM, Lorenzo Stoakes wrote:
> On Thu, Oct 17, 2024 at 02:45:43PM -0700, John Hubbard wrote:
>> On 10/17/24 2:05 PM, Lorenzo Stoakes wrote:
...
>> Your include path above actually refers to:
>>
>>      $(top_srcdir)/include/uapi/linux/fcntl.h
>>
>> ...but what I was intending was to copy a snapshot of that file (or a
>> snapshot from the one generated by "make headers"), to here:
>>
>>      $(top_srcdir)/tools/include/uapi/linux/fcntl.h
> 
> Yeah my first version of this used the uapi one but I thought doing that
> might conflict with snapshotting? Also it'd mean you'd absolutely have to
> have the $(TOOLS_INCLUDES) earlier in the include priority list and better
> maybe to special case in this instance.

Actually, I think the goal is to just stop using KHDR_INCLUDES (./usr/include)
entirely!

More below...

> 
>>
>> ...and then use $(TOOLS_INCLUDES), which is already in selftests/lib.mk,
>> for that reason: to be available to all of the kselftests:
>>
>>      TOOLS_INCLUDES := -isystem $(top_srcdir)/tools/include/uapi
>>
>> The reasoning for this directory is further explained here:
>>
>>      tools/include/uapi/README
>>
>> (And I see that selftests/proc has started using $(TOOLS_INCLUDES), that's
>> progress.)
>>
>> And now, it's possible to change fcntl.h in place, instead of using a wrapper.
>> Although either way seems OK to me. (I'm sort of ignoring the details of
>> the actual header file conflict itself, for now.)
> 
> The fcntl.h and linux/fcntl.h conflict is apparently a rather well-known
> horror show. It's a difficult one to resolve as the UAPI pidfd.h header
> needs O_xxx defines but we also need to include this header in kernel code.
> 
> An #ifdef __KERNEL__ block might be a solution here but fixing that is out
> of scope for these changes.

Certainly out of scope! Your patch already avoids the biggest issue: it no
longer requires "make headers", in order to build it. That's fine for now. Sorry
to put you into the middle of a pre-existing kselftests debate.

And the #ifdef __KERNEL__ sounds like a potential solution, or at least a
building block for one. I need to take a closer look at this particular header
file mess, the fcntl.h situation is new to me.

  
>>> +#endif /* _TOOLS_LINUX_PIDFD_H */
>>> diff --git a/tools/testing/selftests/pidfd/Makefile b/tools/testing/selftests/pidfd/Makefile
>>> index d731e3e76d5b..f5038c9dae14 100644
>>> --- a/tools/testing/selftests/pidfd/Makefile
>>> +++ b/tools/testing/selftests/pidfd/Makefile
>>> @@ -1,8 +1,7 @@
>>>    # SPDX-License-Identifier: GPL-2.0-only
>>> -CFLAGS += -g $(KHDR_INCLUDES) -pthread -Wall
>>> +CFLAGS += -g -isystem $(top_srcdir)/tools/include $(KHDR_INCLUDES) -pthread -Wall
>>
>> Instead, it would look like this, which now mostly matches selftests/mm/Makefile,
>> which is also helpful, because eventually this can be factored into a common
>> piece for all selftests:
>>
>>      CFLAGS += -g -isystem $(KHDR_INCLUDES) $(TOOLS_INCLUDES) -pthread -Wall
>>
>> I apologize for just now noticing this! And these kselftests shouldn't require
>> so much fussing around, I know. But once we get this just right, it will work
>> well and last a long time. :)
> 
> Yeah I know, but this won't work due to the header conflict, I was doing
> this previously.
> 
> Also doing it this way means that uapi snapshot doesn't override the usr/
> one if you have that, which I guess you want?

Actually, given that we want (or should want, so I claim) to build without first
running "make headers", and given that "make headers" populates ./usr/include/,
which in turn is what $(KHDR_INCLUDES) points to, this means that eventually we
should end up with approximately:

     CFLAGS += -g -isystem $(TOOLS_INCLUDES) -pthread -Wall

And I just checked, today's selftests/mm builds just fine, with a similar
diff applied, so I'm not totally crazy:

diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile
index 02e1204971b0..b004a8edcba5 100644
--- a/tools/testing/selftests/mm/Makefile
+++ b/tools/testing/selftests/mm/Makefile
@@ -33,7 +33,7 @@ endif
  # LDLIBS.
  MAKEFLAGS += --no-builtin-rules
  
-CFLAGS = -Wall -I $(top_srcdir) $(EXTRA_CFLAGS) $(KHDR_INCLUDES) $(TOOLS_INCLUDES)
+CFLAGS = -Wall -I $(top_srcdir) $(EXTRA_CFLAGS) $(TOOLS_INCLUDES)
  LDLIBS = -lrt -lpthread -lm
  
  TEST_GEN_FILES = cow



thanks,