On Fri, Sep 4, 2026 at 1:02 AM Thomas Weißschuh <[email protected]> wrote: > > On Thu, Sep 03, 2026 at 01:21:27PM -0700, Bill Wendling wrote: > > On Mon, Aug 31, 2026 at 2:22 AM Thomas Weißschuh > > <[email protected]> wrote: > > > On Thu, Aug 27, 2026 at 12:27:30PM -0700, Bill Wendling wrote: > > > > On Thu, Aug 27, 2026 at 6:37 AM Thomas Weißschuh > > > > <[email protected]> wrote: > > > > > > (...) > > > > > > > > > +config USER_NAMESPACE_KUNIT_TEST > > > > > > + bool "Test user namespace map insertion" if !KUNIT_ALL_TESTS > > > > > > + depends on KUNIT=y > > > > > > > > > > Urgh. > > > > > > > > > ?? What's wrong? It's identical to the conditional for EXEC_KUNIT_TEST: > > > > > > Sorry for this non-descript review comment. > > > > > > > config EXEC_KUNIT_TEST > > > > bool "Build execve tests" if !KUNIT_ALL_TESTS > > > > depends on KUNIT=y > > > > default KUNIT_ALL_TESTS > > > > help > > > > This builds the exec KUnit tests, which tests boundary > > > > conditions > > > > of various aspects of the exec internals. > > > > > > The problem is that KUNIT can be built as module, which would prevent this > > > test from being built. We have include/kunit/visibility.h to export > > > certain > > > symbols only to tests and avoid this issue. > > > But I can see that some maintaines don't like this pattern, so maybe they > > > can > > > chime in at some point. > > > > Bradley commented on this earlier (which is why I mentioned > > EXEC_KUNIT_TEST): > > > > <comment> > > The test is #include'd into user_namespace.c, which is builtin (USER_NS > > is a bool), so =m here still compiles the suite into vmlinux. With > > KUNIT=m that calls kunit symbols that live in a module, and the link > > fails. Make it bool and depend on KUNIT=y, like EXEC_KUNIT_TEST: > > > > bool "KUnit test for user namespace map insertion" if !KUNIT_ALL_TESTS > > depends on USER_NS && KUNIT=y > > </comment> > > > > So there's a conflict and, because I'm not a KUnit guru, I'm not sure > > which way is "best". > > It's subjective. So as mentioned before, the preference of the maintainers > should go into it. The aproach I prefer requires a bit more setup boilerplate > but make the tests usable in more circumstances. > Because user_namespace.c is always built-in (USER_NS is a bool), compiling the test into vmlinux causes linker failures if CONFIG_KUNIT=m. Using the "visibility.h" version also strips static from insert_extent() and sort_idmaps() or exporting internal user namespace functions into the kernel symbol table, which isn't ideal.
> > > > > The extended test below already tests everything the 'base' one does. > > > > > Do we need both? > > > > > > > > > The one below tests the sorting algorithm. > > > > > > It *also* tests the insertion, no? > > > (Especially if the conditional on UID_GID_MAP_MAX_BASE_EXTENTS is removed) > > > > > Correct. So there are three types of tests we should run here: > > > > 1. Insertions and accesses that don't go over the initial extents size. > > 2. Insertions and accesses that do go over the initial extents size. > > 3. Accesses outside of the number of entries. > > > > Test (1) is a "smoke" test, where the struct is tested and no > > sanitizer code is used. > > A "smoke" test is useful when the more complete tests can not be run > regularly. > But here both test cases will always run right after each other. Test (2) is > just as cheap as this one. > > *Not* testing the overflow checking here sounds also weird. Test (2) will > excercise the same code, which is not using the checking, anyways. > I suppose this depends on one's view of how testing should be done. I prefer to have small test cases which directly test a specific feature. Other test cases would still run the same code, but they focus on features. So yes, the same code is ran many times during testing, but that's fine, because testing is meant to test one feature at a time. The benefit of this approach is if a test case fails, it's easier to determine what feature is responsible. > > Test (2) makes sure that we can still go over > > the UID_GID_MAP_MAX_BASE_EXTENTS size and the sanitizer won't > > activate. > > Nice. > > > Test (3) (which I'll add in my next upload) throws a sanitizer exception. > > What is the point of testing this specifically for user namespaces? > Normally we expect a used subsystem to work as advertised. > It is that used subsystem's responsibility to test that it does so. > If there is currently no test that validates __counted_by then it surely > should be created. But not here. > This would directly test that the attribute on the struct field is caught by UBSAN. I'm not sure how we could more directly test it otherwise... -bw
