On Wed, Oct 16, 2024 at 04:38:50PM -0600, Shuah Khan wrote:
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@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.
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.
Right but I'm talking about my local builds in order to test the kernel. See John's response.
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.
OK, but just to note that I am now not doing a PIDFD_SELF series, I'm doing a 'PIDFD_SELF and completely change how pidfd does testing' series.
To me the right thing to do would be to send 2 series and not block this one on this issue.
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> */
Like I said to you above (maybe I wasn't clear?) I tried this and doing this doesn't work for me, as sys/mount.h implicitly includes this header, and we need things from that, so we're just broken.
And I cannot submit a series that literally breaks on my machine obviously.
So simply including this header is a no-go here.
I've provided a workaround above. Also John has suggested using the tools/ directory as previously agreed upon. I could remove the linux/fcntl.h dependency from that and place the header there which is probably the neatest solution.
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.
Right but part of a series introducing a new feature isn't to permanently break tests from working.
And the includes are in that UAPI-exposed header file they're pretty much set in stone or risk breaking userland.
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.
I'm not sure how I'm meant to explore 'this breaks the build on my system'. The sys/mount.h is a deal-breaker, there are things in there we _need_.
thanks, -- Shuah
In any case I think copying the header to the tools/ directory with this linux/fcntl.h in some way stubbed out (we could even stub out fcntl.h there?) is the sensible way forward.
A 'just include the header' is simply not an option as it breaks the tests.