Date: Wed, 19 Jun 2024 00:04:24 +0200
From: David Hildenbrand <david@...hat.com>
To: John Hubbard <jhubbard@...dia.com>,
 Andrew Morton <akpm@...ux-foundation.org>, Jeff Xu <jeffxu@...omium.org>,
 Shuah Khan <shuah@...nel.org>
Cc: Andrei Vagin <avagin@...gle.com>,
 Axel Rasmussen <axelrasmussen@...gle.com>,
 Christian Brauner <brauner@...nel.org>, Kees Cook <kees@...nel.org>,
 Kent Overstreet <kent.overstreet@...ux.dev>,
 "Liam R . Howlett" <Liam.Howlett@...cle.com>,
 Muhammad Usama Anjum <usama.anjum@...labora.com>,
 Peter Xu <peterx@...hat.com>, Rich Felker <dalias@...c.org>,
 linux-mm@...ck.org, linux-kselftest@...r.kernel.org,
 LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 1/6] selftests/mm: mseal, self_elf: fix missing
 __NR_mseal

On 18.06.24 23:29, John Hubbard wrote:
> On 6/18/24 1:54 PM, David Hildenbrand wrote:
>> On 18.06.24 22:14, John Hubbard wrote:
>>> On 6/17/24 11:56 PM, David Hildenbrand wrote:
>>>> On 18.06.24 04:24, John Hubbard wrote:
>>> ...
> ...
>>> I can update the commit description with some of the above, if it helps.
>>
>> I think it will. The main concern I had was that we could be ending up including headers with *wrong* data. As long as (a) it compiles where it's supposed to compile (b) it runs where it's supposed to run, we're good :)
>>
> 
> OK, I've drafted an updated commit description (below), and in order
> to reduce email churn perhaps it's best for me to hold onto it for a
> day or two, while we see how v3 fares in linux-next. (Thanks, Andrew,
> for patching that up with my Makefile fix.)
> 
> Here's the draft:
> selftests/mm: mseal, self_elf: fix missing __NR_mseal
> 
> The selftests/mm build isn't exactly "broken", according to the current
> documentation, which still claims that one must run "make headers",
> before building the kselftests. However, according to the new plan to
> get rid of that requirement [1], they are future-broken: attempting to
> build selftests/mm *without* first running "make headers" will fail due
> to not finding __NR_mseal.
> 
> Therefore, include asm-generic/unistd.h, which has all of the system
> call numbers that are needed, abstracted across the various CPU arches.
> 
> Some explanation in support of this "asm-generic" approach:
> 
> For most user space programs, the header file inclusion behaves as
> per this microblaze example, which comes from David Hildenbrand
> (thanks!) :
> 
>       arch/microblaze/include/asm/unistd.h
>           -> #include <uapi/asm/unistd.h>
> 
>       arch/microblaze/include/uapi/asm/unistd.h
>           -> #include <asm/unistd_32.h>
>           -> Generated during "make headers"
> 
>       usr/include/asm/unistd_32.h is generated via
>       arch/microblaze/kernel/syscalls/Makefile with the syshdr command.
> 
>       So we never end up including asm-generic/unistd.h directly on
>       microblaze... [2]
> 
> However, those programs are installed on a single computer that has a
> single set of asm and kernel headers installed.
> 
> In contrast, the kselftests are quite special, because they must provide
> a set of user space programs that:
> 
>       a) Mostly avoid using the installed (distro) system header files.
> 
>       b) Build (and run) on all supported CPU architectures
> 
>       c) Occasionally use symbols that have so new that they have not
>          yet been included in the distro's header files.
> 
> Doing (a) creates a new problem: how to get a set of cross-platform
> headers that works in all cases.
> 
> Fortunately, asm-generic headers solve that one. Which is why we need to
> use them here--at least, for particularly difficult headers such as
> unistd.h.
> 
> The reason this hasn't really come up yet, is that until now, the
> kselftests requirement (which I'm trying to eventually remove) was that
> "make headers" must first be run. That allowed the selftests to get a
> snapshot of sufficiently new header files that looked just like (and
> conflict with) the installed system headers.
> 
> And as an aside, this is also an improvement over past practices of
> simply open-coding in a single (not per-arch) definition of a new
> symbol, directly into the selftest code.
> 
> [1] commit e076eaca5906 ("selftests: break the dependency upon local
> header files")
> 
> [2] https://lore.kernel.org/all/0b152bea-ccb6-403e-9c57-08ed5e828135@redhat.com/
> 
> Fixes: 4926c7a52de7 ("selftest mm/mseal memory sealing")
> Cc: Jeff Xu <jeffxu@...omium.org>
> Cc: David Hildenbrand <david@...hat.com>
> Signed-off-by: John Hubbard <jhubbard@...dia.com>

Thanks!

Acked-by: David Hildenbrand <david@...hat.com>

-- 
Cheers,

David / dhildenb

Powered by blists - more mailing lists