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.
> > > + 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)
> > > +
> > > +static void test_user_ns_map_insert_extended(struct kunit *test)
> > > +{
> > > + struct uid_gid_map map;
> > > + struct uid_gid_extent extent;
> > > + int i, ret;
> > > +
> > > + memset(&map, 0, sizeof(map));
> > > +
> > > + /* Insert more than UID_GID_MAP_MAX_BASE_EXTENTS (e.g., 10)
> > > elements */
> >
> > When UID_GID_MAP_MAX_BASE_EXTENTS is ever increased, this might not be true
> > anymore.
> > Add an assertion or make the iteration count dynamic.
> >
> Sure, I can make it something like "UID_GID_MAP_MAX_BASE_EXTENTS + 42"
> or something.
That sounds good.
> > > + 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.
Thomas
