Re: [PATCH v3 1/6] selftests/mm: mseal, self_elf: fix missing __NR_mseal

[David Hildenbrand <david@redhat.com>]

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@chromium.org>
> Cc: David Hildenbrand <david@redhat.com>
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>

Thanks!

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

-- 
Cheers,

David / dhildenb