On Wed, Oct 16, 2024 at 04:38:50PM -0600, Shuah Khan wrote:

On 10/16/24 16:06, Lorenzo Stoakes wrote:

quoted

On Wed, Oct 16, 2024 at 02:00:27PM -0600, Shuah Khan wrote:

quoted

On 10/16/24 04:20, Lorenzo Stoakes wrote:

quoted

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 <redacted>
---
   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.

quoted

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.

quoted

Unfortunately this seems broken on my system anyway :( - see below.

quoted

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.

quoted

quoted

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.

quoted

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.

quoted

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.

quoted

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.

quoted

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.