linux-kernel - Re: [PATCH v4 3/4] selftests: pidfd: add pidfd.h UAPI wrapper

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3c27a383-45f9-4307-b1b2-f6bfa78633f2@nvidia.com>
Date: Fri, 18 Oct 2024 16:55:37 -0700
From: John Hubbard <jhubbard@...dia.com>
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>
Subject: Re: [PATCH v4 3/4] selftests: pidfd: add pidfd.h UAPI wrapper

On 10/17/24 11:49 PM, Lorenzo Stoakes wrote:
> On Thu, Oct 17, 2024 at 02:45:43PM -0700, John Hubbard wrote:
>> On 10/17/24 2:05 PM, Lorenzo Stoakes wrote:
...
>> Your include path above actually refers to:
>>
>>      $(top_srcdir)/include/uapi/linux/fcntl.h
>>
>> ...but what I was intending was to copy a snapshot of that file (or a
>> snapshot from the one generated by "make headers"), to here:
>>
>>      $(top_srcdir)/tools/include/uapi/linux/fcntl.h
> 
> Yeah my first version of this used the uapi one but I thought doing that
> might conflict with snapshotting? Also it'd mean you'd absolutely have to
> have the $(TOOLS_INCLUDES) earlier in the include priority list and better
> maybe to special case in this instance.

Actually, I think the goal is to just stop using KHDR_INCLUDES (./usr/include)
entirely!

More below...

> 
>>
>> ...and then use $(TOOLS_INCLUDES), which is already in selftests/lib.mk,
>> for that reason: to be available to all of the kselftests:
>>
>>      TOOLS_INCLUDES := -isystem $(top_srcdir)/tools/include/uapi
>>
>> The reasoning for this directory is further explained here:
>>
>>      tools/include/uapi/README
>>
>> (And I see that selftests/proc has started using $(TOOLS_INCLUDES), that's
>> progress.)
>>
>> And now, it's possible to change fcntl.h in place, instead of using a wrapper.
>> Although either way seems OK to me. (I'm sort of ignoring the details of
>> the actual header file conflict itself, for now.)
> 
> The fcntl.h and linux/fcntl.h conflict is apparently a rather well-known
> horror show. It's a difficult one to resolve as the UAPI pidfd.h header
> needs O_xxx defines but we also need to include this header in kernel code.
> 
> An #ifdef __KERNEL__ block might be a solution here but fixing that is out
> of scope for these changes.

Certainly out of scope! Your patch already avoids the biggest issue: it no
longer requires "make headers", in order to build it. That's fine for now. Sorry
to put you into the middle of a pre-existing kselftests debate.

And the #ifdef __KERNEL__ sounds like a potential solution, or at least a
building block for one. I need to take a closer look at this particular header
file mess, the fcntl.h situation is new to me.

  
>>> +#endif /* _TOOLS_LINUX_PIDFD_H */
>>> diff --git a/tools/testing/selftests/pidfd/Makefile b/tools/testing/selftests/pidfd/Makefile
>>> index d731e3e76d5b..f5038c9dae14 100644
>>> --- a/tools/testing/selftests/pidfd/Makefile
>>> +++ b/tools/testing/selftests/pidfd/Makefile
>>> @@ -1,8 +1,7 @@
>>>    # SPDX-License-Identifier: GPL-2.0-only
>>> -CFLAGS += -g $(KHDR_INCLUDES) -pthread -Wall
>>> +CFLAGS += -g -isystem $(top_srcdir)/tools/include $(KHDR_INCLUDES) -pthread -Wall
>>
>> Instead, it would look like this, which now mostly matches selftests/mm/Makefile,
>> which is also helpful, because eventually this can be factored into a common
>> piece for all selftests:
>>
>>      CFLAGS += -g -isystem $(KHDR_INCLUDES) $(TOOLS_INCLUDES) -pthread -Wall
>>
>> I apologize for just now noticing this! And these kselftests shouldn't require
>> so much fussing around, I know. But once we get this just right, it will work
>> well and last a long time. :)
> 
> Yeah I know, but this won't work due to the header conflict, I was doing
> this previously.
> 
> Also doing it this way means that uapi snapshot doesn't override the usr/
> one if you have that, which I guess you want?

Actually, given that we want (or should want, so I claim) to build without first
running "make headers", and given that "make headers" populates ./usr/include/,
which in turn is what $(KHDR_INCLUDES) points to, this means that eventually we
should end up with approximately:

     CFLAGS += -g -isystem $(TOOLS_INCLUDES) -pthread -Wall

And I just checked, today's selftests/mm builds just fine, with a similar
diff applied, so I'm not totally crazy:

diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile
index 02e1204971b0..b004a8edcba5 100644
--- a/tools/testing/selftests/mm/Makefile
+++ b/tools/testing/selftests/mm/Makefile
@@ -33,7 +33,7 @@ endif
  # LDLIBS.
  MAKEFLAGS += --no-builtin-rules
  
-CFLAGS = -Wall -I $(top_srcdir) $(EXTRA_CFLAGS) $(KHDR_INCLUDES) $(TOOLS_INCLUDES)
+CFLAGS = -Wall -I $(top_srcdir) $(EXTRA_CFLAGS) $(TOOLS_INCLUDES)
  LDLIBS = -lrt -lpthread -lm
  
  TEST_GEN_FILES = cow



thanks,
-- 
John Hubbard