| Message ID | 20240618022422.804305-1-jhubbard@nvidia.com |
|---|---|
| Headers | show |
| Series | cleanups, fixes, and progress towards avoiding "make headers" | expand |
On 6/17/24 7:24 PM, John Hubbard wrote: > Clean up and move some copy-pasted items into a new mseal_helpers.h. > > 1. The test macros can be made safer and simpler, by observing that they > are invariably called when about to return. This means that the macros > do not need an intrusive label to goto; they can simply return. > > 2. PKEY* items. We cannot, unfortunately use pkey-helpers.h. The best we > can do is to factor out these few items into mseal_helpers.h. > > 3. These tests still need their own definition of u64, so also move that > to the header file. And I just noticed that I left out this one: 4. Be sure to include the new mseal_helpers.h in the Makefile dependencies. In other words, this hunk is also needed: diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile index 3b49bc3d0a3b..23daa097d5b7 100644 --- a/tools/testing/selftests/mm/Makefile +++ b/tools/testing/selftests/mm/Makefile @@ -2,6 +2,7 @@ # Makefile for mm selftests LOCAL_HDRS += $(selfdir)/mm/local_config.h $(top_srcdir)/mm/gup_test.h +LOCAL_HDRS += $(selfdir)/mm/mseal_helpers.h include local_config.mk I'll send out a v4 with that, once we resolve the discussion around patch 1/6. thanks,
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,
On Tue, 18 Jun 2024 14:29:32 -0700 John Hubbard <jhubbard@nvidia.com> wrote:
> OK, I've drafted an updated commit description (below)
<copy><paste>
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>
Changes since v2: a) After some disussion with David Hildenbrand, simplified the "fix missing __NR_mseal" patch down to just two lines of diff, after all. b) Improved the "kvm, mdwe fixes to avoid requiring "make headers"" patch by taking a snapshot of the prctl.h, instead of manually creating defines. Thanks also to David Hildenbrand for that. c) Fixed up the final patch to use asm-generic/unistd.h. d) Added acks. e) Rebased onto 6.10-rc4+ (today's top of tree). Changes since v1: Jeff Xu, I apologize for this churn: I was forced to drop your Reviewed-by and Tested-by tags from 2 of the 3 mseal patches, because the __NR_mseal fix is completely different now. a) Reworked the mseal fix to use the kernel's in-tree unistd*.h files, instead of hacking in a __NR_mseal definition directly. (Thanks to David Hildenbrand for pointing out that this needed to be done.) b) Fixed the subject line of the kvm and mdwe patch. c) Reordered the patches so as to group the mseal changes together. d) ADDED an additional patch, 6/6, to remove various __NR_xx items and checks from the mm selftests. Cover letter, updated for v2: Eventually, once the build succeeds on a sufficiently old distro, the idea is to delete $(KHDR_INCLUDES) from the selftests/mm build, and then after that, from selftests/lib.mk and all of the other selftest builds. For now, this series merely achieves a clean build of selftests/mm on a not-so-old distro: Ubuntu 23.04. In other words, after this series is applied, it is possible to delete $(KHDR_INCLUDES) from selftests/mm/Makefile and the build will still succeed. 1. Add tools/uapi/asm/unistd_[32|x32|64].h files, which include definitions of __NR_mseal, and include them (indirectly) from the files that use __NR_mseal. The new files are copied from ./usr/include/asm, which is how we have agreed to do this sort of thing, see [1]. 2. Add fs.h, similarly created: it was copied directly from a snapshot of ./usr/include/linux/fs.h after running "make headers". 3. Add a few selected prctl.h values that the ksm and mdwe tests require. 4. Factor out some common code from mseal_test.c and seal_elf.c, into a new mseal_helpers.h file. 5. Remove local __NR_* definitions and checks. [1] commit e076eaca5906 ("selftests: break the dependency upon local header files") John Hubbard (6): selftests/mm: mseal, self_elf: fix missing __NR_mseal selftests/mm: mseal, self_elf: factor out test macros and other duplicated items selftests/mm: mseal, self_elf: rename TEST_END_CHECK to REPORT_TEST_PASS selftests/mm: fix vm_util.c build failures: add snapshot of fs.h selftests/mm: kvm, mdwe fixes to avoid requiring "make headers" selftests/mm: remove local __NR_* definitions tools/include/uapi/linux/fs.h | 392 ++++++++++++++++++ tools/include/uapi/linux/prctl.h | 331 +++++++++++++++ tools/testing/selftests/mm/hugepage-mremap.c | 2 +- .../selftests/mm/ksm_functional_tests.c | 8 +- tools/testing/selftests/mm/memfd_secret.c | 14 +- tools/testing/selftests/mm/mkdirty.c | 8 +- tools/testing/selftests/mm/mlock2.h | 1 + tools/testing/selftests/mm/mseal_helpers.h | 41 ++ tools/testing/selftests/mm/mseal_test.c | 143 +++---- tools/testing/selftests/mm/pagemap_ioctl.c | 2 +- tools/testing/selftests/mm/protection_keys.c | 2 +- tools/testing/selftests/mm/seal_elf.c | 37 +- tools/testing/selftests/mm/uffd-common.c | 4 - tools/testing/selftests/mm/uffd-stress.c | 16 +- tools/testing/selftests/mm/uffd-unit-tests.c | 14 +- 15 files changed, 824 insertions(+), 191 deletions(-) create mode 100644 tools/include/uapi/linux/fs.h create mode 100644 tools/include/uapi/linux/prctl.h create mode 100644 tools/testing/selftests/mm/mseal_helpers.h base-commit: 14d7c92f8df9c0964ae6f8b813c1b3ac38120825