Message-ID: <a6133831-3fc3-49aa-83c6-f9aeef3713c9@lucifer.local>
Date: Wed, 16 Oct 2024 23:06:34 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Shuah Khan <skhan@...uxfoundation.org>
Cc: Christian Brauner <christian@...uner.io>, Shuah Khan <shuah@...nel.org>,
        "Liam R . Howlett" <Liam.Howlett@...cle.com>,
        Suren Baghdasaryan <surenb@...gle.com>,
        Vlastimil Babka <vbabka@...e.cz>, pedro.falcato@...il.com,
        linux-kselftest@...r.kernel.org, linux-mm@...ck.org,
        linux-fsdevel@...r.kernel.org, linux-api@...r.kernel.org,
        linux-kernel@...r.kernel.org, Oliver Sang <oliver.sang@...el.com>
Subject: Re: [PATCH v3 3/3] selftests: pidfd: add tests for PIDFD_SELF_*

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@...cle.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

Powered by blists - more mailing lists