Message-ID: <6080fb15-9073-461c-a87d-80e6daa326e6@lucifer.local>
Date: Thu, 17 Oct 2024 09:08:19 +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 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@...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.
>
> 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.

Powered by blists - more mailing lists