Hi Thomas,
On Mon, Aug 31, 2026 at 2:22 AM Thomas Weißschuh
<[email protected]> wrote:
>
> Hi Bill,
>
> 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".
> > > > + 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. Test (2) makes sure that we can still go over
the UID_GID_MAP_MAX_BASE_EXTENTS size and the sanitizer won't
activate. Test (3) (which I'll add in my next upload) throws a
sanitizer exception.
> > > > + for (i = 0; i < 10; i++) {
> > > > + int value = 9 - i;
> > > > +
> > > > + extent.first = value * 10;
> > > > + extent.lower_first = value * 100;
> > > > + extent.count = 5;
> > > > +
> > > > + ret = insert_extent(&map, &extent);
> > > > + KUNIT_ASSERT_EQ(test, ret, 0);
> > > > + KUNIT_EXPECT_EQ(test, map.nr_extents, i + 1);
> > > > +
> > > > + if (i < UID_GID_MAP_MAX_BASE_EXTENTS) {
> > > > + KUNIT_EXPECT_EQ(test, map.extent[i].first, value
> > > > * 10);
> > > > + } else {
> > > > + KUNIT_EXPECT_EQ(test, map.forward[i].first, value
> > > > * 10);
> > > > + KUNIT_EXPECT_EQ(test, map.forward[i].lower_first,
> > > > value * 100);
> > > > + KUNIT_EXPECT_EQ(test, map.forward[i].count, 5);
> > > > + }
> > > > + }
> > > > +
> > > > + /* Now sort the map to set up reverse mapping */
> > > > + ret = sort_idmaps(&map);
> > > > + KUNIT_EXPECT_EQ(test, ret, 0);
> > > > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, map.reverse);
> > > > +
> > > > + /* Verify sorting is correct */
> > > > + for (i = 0; i < map.nr_extents; i++) {
> > > > + KUNIT_EXPECT_EQ(test, map.forward[i].first, i * 10);
> > > > + KUNIT_EXPECT_EQ(test, map.forward[i].lower_first, i *
> > > > 100);
> > > > + KUNIT_EXPECT_EQ(test, map.forward[i].count, 5);
> > > > +
> > > > + KUNIT_EXPECT_EQ(test, map.reverse[i].first, i * 10);
> > > > + KUNIT_EXPECT_EQ(test, map.reverse[i].lower_first, i *
> > > > 100);
> > > > + KUNIT_EXPECT_EQ(test, map.reverse[i].count, 5);
> > > > + }
> > >
> > > Isn't this the same as the original order?
> > >
> > No. The original order uses "9 - i" for the base value, so it's not in
> > sorted order (though it's not exactly random either).
>
> Indeed, sorry for missing this.
> If you send a new revision, maybe add a small comment.
>
Ack.
-bw
