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@oracle.com
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.
This is a use case I use all the time so not at all theoretical.
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.
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
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.
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.
thanks, -- Shuah