Re: [PATCH 3/3] selftests: pidfd: add tests for PIDFD_SELF_*
On 10/11/24 02:20, Lorenzo Stoakes wrote: On Thu, Oct 10, 2024 at 05:16:22PM -0600, Shuah Khan wrote: On 10/10/24 12:15, Lorenzo Stoakes wrote: Add tests to assert that PIDFD_SELF_* correctly refers to the current thread and process. This is only practically meaningful to pidfd_send_signal() and pidfd_getfd(), but also explicitly test that we disallow this feature for setns() where it would make no sense. We cannot reasonably wait on ourself using waitid(P_PIDFD, ...) so while in theory PIDFD_SELF_* would work here, we'd be left blocked if we tried it. We defer testing of mm-specific functionality which uses pidfd, namely process_madvise() and process_mrelease() to mm testing (though note the latter can not be sensibly tested as it would require the testing process to be dying). Signed-off-by: Lorenzo Stoakes --- tools/testing/selftests/pidfd/pidfd.h | 8 ++ .../selftests/pidfd/pidfd_getfd_test.c| 136 ++ .../selftests/pidfd/pidfd_setns_test.c| 11 ++ tools/testing/selftests/pidfd/pidfd_test.c| 67 +++-- 4 files changed, 213 insertions(+), 9 deletions(-) diff --git a/tools/testing/selftests/pidfd/pidfd.h b/tools/testing/selftests/pidfd/pidfd.h index 88d6830ee004..1640b711889b 100644 --- a/tools/testing/selftests/pidfd/pidfd.h +++ b/tools/testing/selftests/pidfd/pidfd.h @@ -50,6 +50,14 @@ #define PIDFD_NONBLOCK O_NONBLOCK #endif +/* System header file may not have this available. */ +#ifndef PIDFD_SELF_THREAD +#define PIDFD_SELF_THREAD -100 +#endif +#ifndef PIDFD_SELF_THREAD_GROUP +#define PIDFD_SELF_THREAD_GROUP -200 +#endif + Can't we pick these up from linux/pidfd.h - patch 2/3 adds them. We're running this file in userland and it's not obvious we can correctly import this header, it'd be some "../../" thing out of the testing root directory and might not interact well with all scenarios in which this file is built. Also the existing tests do not seem to try to import that header, so it seemed the safest way of doing this. kselftest has dependency on "make headers" and tests include headers from linux/ directory These local make it difficult to maintain these tests in the longer term. Somebody has to go clean these up later. The import will be fine and you can control that with -I flag in the makefile. Remove these and try to get including linux/pidfd.h working. I see your v2 and v3. Please revise this patch to include the header file and remove these local defines. thanks, -- Shuah
Re: [PATCH 3/3] selftests: pidfd: add tests for PIDFD_SELF_*
On Thu, Oct 10, 2024 at 05:16:22PM -0600, Shuah Khan wrote:
> On 10/10/24 12:15, Lorenzo Stoakes wrote:
> > Add tests to assert that PIDFD_SELF_* correctly refers to the current
> > thread and process.
> >
> > This is only practically meaningful to pidfd_send_signal() and
> > pidfd_getfd(), but also explicitly test that we disallow this feature for
> > setns() where it would make no sense.
> >
> > We cannot reasonably wait on ourself using waitid(P_PIDFD, ...) so while in
> > theory PIDFD_SELF_* would work here, we'd be left blocked if we tried it.
> >
> > We defer testing of mm-specific functionality which uses pidfd, namely
> > process_madvise() and process_mrelease() to mm testing (though note the
> > latter can not be sensibly tested as it would require the testing process
> > to be dying).
> >
> > Signed-off-by: Lorenzo Stoakes
> > ---
> > tools/testing/selftests/pidfd/pidfd.h | 8 ++
> > .../selftests/pidfd/pidfd_getfd_test.c| 136 ++
> > .../selftests/pidfd/pidfd_setns_test.c| 11 ++
> > tools/testing/selftests/pidfd/pidfd_test.c| 67 +++--
> > 4 files changed, 213 insertions(+), 9 deletions(-)
> >
> > diff --git a/tools/testing/selftests/pidfd/pidfd.h
> > b/tools/testing/selftests/pidfd/pidfd.h
> > index 88d6830ee004..1640b711889b 100644
> > --- a/tools/testing/selftests/pidfd/pidfd.h
> > +++ b/tools/testing/selftests/pidfd/pidfd.h
> > @@ -50,6 +50,14 @@
> > #define PIDFD_NONBLOCK O_NONBLOCK
> > #endif
> > +/* System header file may not have this available. */
> > +#ifndef PIDFD_SELF_THREAD
> > +#define PIDFD_SELF_THREAD -100
> > +#endif
> > +#ifndef PIDFD_SELF_THREAD_GROUP
> > +#define PIDFD_SELF_THREAD_GROUP -200
> > +#endif
> > +
>
> Can't we pick these up from linux/pidfd.h - patch 2/3 adds
> them.
We're running this file in userland and it's not obvious we can correctly
import this header, it'd be some "../../" thing out of the testing root
directory and might not interact well with all scenarios in which this file
is built.
Also the existing tests do not seem to try to import that header, so it
seemed the safest way of doing this.
>
> > /*
> >* The kernel reserves 300 pids via RESERVED_PIDS in kernel/pid.c
> >* That means, when it wraps around any pid < 300 will be skipped.
> > diff --git a/tools/testing/selftests/pidfd/pidfd_getfd_test.c
> > b/tools/testing/selftests/pidfd/pidfd_getfd_test.c
> > index cd51d547b751..10793fc845ed 100644
> > --- a/tools/testing/selftests/pidfd/pidfd_getfd_test.c
> > +++ b/tools/testing/selftests/pidfd/pidfd_getfd_test.c
> > @@ -6,6 +6,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -15,6 +16,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > @@ -114,6 +116,89 @@ static int child(int sk)
> > return ret;
> > }
> > +static int __pidfd_self_thread_worker(unsigned long page_size)
> > +{
> > + int memfd;
> > + int newfd;
> > + char *ptr;
> > + int ret = 0;
> > +
> > + /*
> > +* Unshare our FDs so we have our own set. This means
> > +* PIDFD_SELF_THREAD_GROUP will fail.
> > +*/
> > + if (unshare(CLONE_FILES) < 0) {
> > + ret = -errno;
> > + goto exit;
> > + }
> > +
> > + /* Truncate, map in and write to our memfd. */
> > + memfd = sys_memfd_create("test_self_child", 0);
>
> Missing eror check.
Ack, I had to rapidly change this code to not use the ASSERT_xxx() stuff
since abstracted out to helper function for pthread() to invoke and clearly
did not do so carefully enough :) thanks for pointing out will fix this +
other issues.
>
> > + if (ftruncate(memfd, page_size)) {
> > + ret = -errno;
> > + goto exit;
>
> Hmm. you probably need scoped cleanup paths. "exit" closes
> memfd and newfd which isn't open yet and sys_memfd_create()
> could fail and memfd doesn't need closing?
Yes... oops! Will fix.
>
> > + }
> > +
> > + ptr = mmap(NULL, page_size, PROT_READ | PROT_WRITE,
> > + MAP_SHARED, memfd, 0);
> > + if (ptr == MAP_FAILED) {
> > + ret = -errno;
> > + goto exit;
> > + }
> > + ptr[0] = 'y';
> > + if (munmap(ptr, page_size)) {
> > + ret = -errno;
> > + goto exit;
> > + }
> > +
> > + /* Get a thread-local duplicate of our memfd. */
> > + newfd = sys_pidfd_getfd(PIDFD_SELF_THREAD, memfd, 0);
> > + if (newfd < 0) {
> > + ret = -errno;
> > + goto exit;
>
> Same comment here - "exit" closes newfd
Ack will fix.
>
> > + }
> > +
> > + if (memfd == newfd) {
> > + ret = -EINVAL;
> > + goto exit;
> > + }
> > +
> > + /* Map in new fd and make sure that the data is as expected. */
> > + ptr = mmap(NULL, page_size, PROT_READ | PROT_WRITE,
> > + MAP_SHARED, newfd, 0);
> > + if (ptr == MAP_FAILED) {
> > + ret = -errno;
> > + goto exit;
> > + }
>
Re: [PATCH 3/3] selftests: pidfd: add tests for PIDFD_SELF_*
On 10/10/24 12:15, Lorenzo Stoakes wrote:
Add tests to assert that PIDFD_SELF_* correctly refers to the current
thread and process.
This is only practically meaningful to pidfd_send_signal() and
pidfd_getfd(), but also explicitly test that we disallow this feature for
setns() where it would make no sense.
We cannot reasonably wait on ourself using waitid(P_PIDFD, ...) so while in
theory PIDFD_SELF_* would work here, we'd be left blocked if we tried it.
We defer testing of mm-specific functionality which uses pidfd, namely
process_madvise() and process_mrelease() to mm testing (though note the
latter can not be sensibly tested as it would require the testing process
to be dying).
Signed-off-by: Lorenzo Stoakes
---
tools/testing/selftests/pidfd/pidfd.h | 8 ++
.../selftests/pidfd/pidfd_getfd_test.c| 136 ++
.../selftests/pidfd/pidfd_setns_test.c| 11 ++
tools/testing/selftests/pidfd/pidfd_test.c| 67 +++--
4 files changed, 213 insertions(+), 9 deletions(-)
diff --git a/tools/testing/selftests/pidfd/pidfd.h
b/tools/testing/selftests/pidfd/pidfd.h
index 88d6830ee004..1640b711889b 100644
--- a/tools/testing/selftests/pidfd/pidfd.h
+++ b/tools/testing/selftests/pidfd/pidfd.h
@@ -50,6 +50,14 @@
#define PIDFD_NONBLOCK O_NONBLOCK
#endif
+/* System header file may not have this available. */
+#ifndef PIDFD_SELF_THREAD
+#define PIDFD_SELF_THREAD -100
+#endif
+#ifndef PIDFD_SELF_THREAD_GROUP
+#define PIDFD_SELF_THREAD_GROUP -200
+#endif
+
Can't we pick these up from linux/pidfd.h - patch 2/3 adds
them.
/*
* The kernel reserves 300 pids via RESERVED_PIDS in kernel/pid.c
* That means, when it wraps around any pid < 300 will be skipped.
diff --git a/tools/testing/selftests/pidfd/pidfd_getfd_test.c
b/tools/testing/selftests/pidfd/pidfd_getfd_test.c
index cd51d547b751..10793fc845ed 100644
--- a/tools/testing/selftests/pidfd/pidfd_getfd_test.c
+++ b/tools/testing/selftests/pidfd/pidfd_getfd_test.c
@@ -6,6 +6,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -15,6 +16,7 @@
#include
#include
#include
+#include
#include
#include
@@ -114,6 +116,89 @@ static int child(int sk)
return ret;
}
+static int __pidfd_self_thread_worker(unsigned long page_size)
+{
+ int memfd;
+ int newfd;
+ char *ptr;
+ int ret = 0;
+
+ /*
+* Unshare our FDs so we have our own set. This means
+* PIDFD_SELF_THREAD_GROUP will fail.
+*/
+ if (unshare(CLONE_FILES) < 0) {
+ ret = -errno;
+ goto exit;
+ }
+
+ /* Truncate, map in and write to our memfd. */
+ memfd = sys_memfd_create("test_self_child", 0);
Missing eror check.
+ if (ftruncate(memfd, page_size)) {
+ ret = -errno;
+ goto exit;
Hmm. you probably need scoped cleanup paths. "exit" closes
memfd and newfd which isn't open yet and sys_memfd_create()
could fail and memfd doesn't need closing?
+ }
+
+ ptr = mmap(NULL, page_size, PROT_READ | PROT_WRITE,
+ MAP_SHARED, memfd, 0);
+ if (ptr == MAP_FAILED) {
+ ret = -errno;
+ goto exit;
+ }
+ ptr[0] = 'y';
+ if (munmap(ptr, page_size)) {
+ ret = -errno;
+ goto exit;
+ }
+
+ /* Get a thread-local duplicate of our memfd. */
+ newfd = sys_pidfd_getfd(PIDFD_SELF_THREAD, memfd, 0);
+ if (newfd < 0) {
+ ret = -errno;
+ goto exit;
Same comment here - "exit" closes newfd
+ }
+
+ if (memfd == newfd) {
+ ret = -EINVAL;
+ goto exit;
+ }
+
+ /* Map in new fd and make sure that the data is as expected. */
+ ptr = mmap(NULL, page_size, PROT_READ | PROT_WRITE,
+ MAP_SHARED, newfd, 0);
+ if (ptr == MAP_FAILED) {
+ ret = -errno;
+ goto exit;
+ }
+
+ if (ptr[0] != 'y') {
+ ret = -EINVAL;
+ goto exit;
+ }
+
+ if (munmap(ptr, page_size)) {
+ ret = -errno;
+ goto exit;
+ }
+
+exit:
+ /* Cleanup. */
+ close(newfd);
+ close(memfd);
+
+ return ret;
+}
+
+static void *pidfd_self_thread_worker(void *arg)
+{
+ unsigned long page_size = (unsigned long)arg;
+ int ret;
+
+ ret = __pidfd_self_thread_worker(page_size);
Don't you want to check error here?
+
+ return (void *)(intptr_t)ret;
+}
+
FIXTURE(child)
{
/*
@@ -264,6 +349,57 @@ TEST_F(child, no_strange_EBADF)
EXPECT_EQ(errno, ESRCH);
}
+TEST(pidfd_self)
+{
+ int memfd = sys_memfd_create("test_self", 0);
+ unsigned long page_size = sysconf(_SC_PAGESIZE);
+ int newfd;
+ char *ptr;
+ pthread_t thread;
+ void *res;
+ int err;
+
+ ASSERT_GE(memfd, 0);
+
