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.
> > > > > + default KUNIT_ALL_TESTS
> > > > > + help
> > > > > + This builds the KUnit test for user namespace uid/gid map
> > > > > insertion.
> > > > > + It validates map insertion, limits, dynamic allocation of the
> > > > > + extended extents array, and mapping sorting functions.
> > > > > + If unsure, say N.
> > > > > +
> > > > > config PID_NS
> > > > > bool "PID Namespaces"
> > > > > default y
> > > > > diff --git a/kernel/.kunitconfig b/kernel/.kunitconfig
> > > > > new file mode 100644
> > > > > index 000000000000..7314dce05dc2
> > > > > --- /dev/null
> > > > > +++ b/kernel/.kunitconfig
> > > > > @@ -0,0 +1,3 @@
> > > > > +CONFIG_KUNIT=y
> > > > > +CONFIG_USER_NS=y
> > > > > +CONFIG_USER_NAMESPACE_KUNIT_TEST=y
> > > > > diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
> > > > > index 786dbf0506ca..0e7373085af9 100644
> > > > > --- a/kernel/user_namespace.c
> > > > > +++ b/kernel/user_namespace.c
> > > > > @@ -1417,3 +1417,7 @@ static __init int user_namespaces_init(void)
> > > > > return 0;
> > > > > }
> > > > > subsys_initcall(user_namespaces_init);
> > > > > +
> > > > > +#if IS_ENABLED(CONFIG_USER_NAMESPACE_KUNIT_TEST)
> > > > > +#include "user_namespace_kunit.c"
> > > > > +#endif
> > > > > diff --git a/kernel/user_namespace_kunit.c
> > > > > b/kernel/user_namespace_kunit.c
> > > > > new file mode 100644
> > > > > index 000000000000..88467361efdf
> > > > > --- /dev/null
> > > > > +++ b/kernel/user_namespace_kunit.c
> > > > > @@ -0,0 +1,92 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > +/*
> > > > > + * KUnit test for user namespace map insertion and sorting.
> > > > > + */
> > > > > +
> > > > > +#include <kunit/test.h>
> > > > > +#include <linux/user_namespace.h>
> > > > > +
> > > > > +static void test_user_ns_map_insert_base(struct kunit *test)
> > > > > +{
> > > > > + struct uid_gid_map map;
> > > > > + struct uid_gid_extent extent;
> > > > > + int i, ret;
> > > > > +
> > > > > + memset(&map, 0, sizeof(map));
> > > > > +
> > > > > + /* Insert up to UID_GID_MAP_MAX_BASE_EXTENTS (5) elements */
> > > > > + for (i = 0; i < UID_GID_MAP_MAX_BASE_EXTENTS; i++) {
> > > > > + extent.first = i * 10;
> > > > > + extent.lower_first = i * 100;
> > > > > + extent.count = 5;
> > > > > +
> > > > > + ret = insert_extent(&map, &extent);
> > > > > + KUNIT_ASSERT_EQ(test, ret, 0);
> > > > > + KUNIT_EXPECT_EQ(test, map.nr_extents, i + 1);
> > > > > + KUNIT_EXPECT_EQ(test, map.extent[i].first, i * 10);
> > > > > + KUNIT_EXPECT_EQ(test, map.extent[i].lower_first, i *
> > > > > 100);
> > > > > + KUNIT_EXPECT_EQ(test, map.extent[i].count, 5);
> > > > > + }
> > > > > +}
> > > >
> > > > 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.
> 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.
Thomas
