Message-ID: <84c0de17-899e-46fd-8b72-534d8a02c259@linuxfoundation.org>
Date: Wed, 16 Oct 2024 16:38:50 -0600
From: Shuah Khan <skhan@...uxfoundation.org>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
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>, Shuah Khan <skhan@...uxfoundation.org>
Subject: Re: [PATCH v3 3/3] selftests: pidfd: add tests for PIDFD_SELF_*

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.

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

> 
>>
>> 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> */

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

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

thanks,
-- Shuah

Powered by blists - more mailing lists