Re: [PATCH v3 3/3] selftests: pidfd: add tests for PIDFD_SELF_*
From: Shuah Khan
Date: Wed Oct 16 2024 - 18:39:09 EST
On 10/16/24 16:06, Lorenzo Stoakes wrote:
On Wed, Oct 16, 2024 at 02:00:27PM -0600, Shuah Khan wrote:
On 10/16/24 04:20, 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 <lorenzo.stoakes@xxxxxxxxxx>
---
tools/testing/selftests/pidfd/pidfd.h | 8 +
.../selftests/pidfd/pidfd_getfd_test.c | 141 ++++++++++++++++++
.../selftests/pidfd/pidfd_setns_test.c | 11 ++
tools/testing/selftests/pidfd/pidfd_test.c | 76 ++++++++--
4 files changed, 224 insertions(+), 12 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
+
As mentioned in my response to v1 patch:
kselftest has dependency on "make headers" and tests include
headers from linux/ directory
Right but that assumes you install the kernel headers on the build system,
which is quite a painful thing to have to do when you are quickly iterating
on a qemu setup.
Yes that is exactly what we do. kselftest build depends on headers
install. The way it works for qemu is either using vitme-ng or
building tests and installing them in your vm.. This is what CIs do.
This is a use case I use all the time so not at all theoretical.
This is what CIs do. Yes - it works for them to build and install
headers. You don't have to install them on the build system. You
run "make headers" in your repo. You could use O= option for
relocatable build.
Unfortunately this seems broken on my system anyway :( - see below.
These local make it difficult to maintain these tests in the
longer term. Somebody has to go clean these up later.
I don't agree, tests have to be maintained alongside the core code, and if
these values change (seems unlikely) then the tests will fail and can
easily be updated.
This was the approach already taken in this file with other linux
header-defined values, so we'll also be breaking the precendence.
Some of these defines were added a while back. Often these defines
need cleaning up. I would rather not see new ones added unless it is
absolutely necessary.
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 just tried this and it's not fine :) it immediately broke the build as
pidfd.h imports linux/fcntl.h which conflicts horribly with system headers
on my machine.
For instance f_owner_ex gets redefined among others and fails the build e..g:
/usr/include/asm-generic/fcntl.h:155:8: error: redefinition of ‘struct f_owner_ex’
155 | struct f_owner_ex {
| ^~~~~~~~~~
In file included from /usr/include/bits/fcntl.h:61,
from /usr/include/fcntl.h:35,
from pidfd_test.c:6:
/usr/include/bits/fcntl-linux.h:274:8: note: originally defined here
274 | struct f_owner_ex
| ^~~~~~~~~~
It seems only one other test tries to do this as far as I can tell (I only
did a quick grep), so it's not at all standard it seems.
This issue occurred even when I used make headers_install to create
sanitised user headers and added them to the include path.
A quick google suggests linux/fcntl.h (imported by this pidfd.h uapi
header) and system fcntl.h is a known thing. Slightly bizarre...
I tried removing the <fcntl.h> include and that resulted in <sys/mount.h>
conflicting:
In file included from /usr/include/fcntl.h:35,
from /usr/include/sys/mount.h:24,
from pidfd.h:17,
from pidfd_test.c:22:
/usr/include/bits/fcntl.h:35:8: error: redefinition of ‘struct flock’
35 | struct flock
| ^~~~~
In file included from /tmp/hdr/include/asm/fcntl.h:1,
from /tmp/hdr/include/linux/fcntl.h:5,
from /tmp/hdr/include/linux/pidfd.h:7,
from pidfd.h:6:
/usr/include/asm-generic/fcntl.h:195:8: note: originally defined here
195 | struct flock {
| ^~~~~
So I don't think I can actually work around this, at least on my system,
and I can't really sensibly submit a patch that I can't run on my own
machine :)
I may be missing something here.
Please revise this patch to include the header file and remove
these local defines.
I'm a little stuck because of the above, but I _could_ do the following in
the test pidfd.h header.:
#define _LINUX_FCNTL_H
#include "../../../../include/uapi/linux/pidfd.h"
#undef _LINUX_FCNTL_H
Does this test really need fcntl.h is another question.
This is another problem with too many includes. The test
built just fine on my system on 6.12-rc3 with
+/* #include <fcntl.h> */
Which prevents the problematic linux/fcntl.h header from being included and
includes the right header.
But I'm not sure this is hugely better than what we already have
maintinability-wise? Either way if something changes to break it it'll
break the test build.
If these defines are in a header file - tests include them. Part
of test development is figuring out these problems.
Let me know if this is what you want me to do. Otherwise I'm not sure how
to proceed - this header just seems broken at least on my system (arch
linux at 6.11.1).
An aside:
The existing code already taken the approach I take (this is partly why I
did it), I think it'd be out of the scope of my series to change that, for
instance in pidfd.h:
#ifndef PIDFD_NONBLOCK
#define PIDFD_NONBLOCK O_NONBLOCK
#endif
Alongside a number of other defines. So those will have to stay at least
for now for being out of scope, but obviously if people would prefer to
move the whole thing that can be followed up later.
I would like us to explore before giving up and saying these will
stay.
thanks,
-- Shuah