On Fri, Sep 4, 2026 at 4:34 PM Kees Cook <[email protected]> wrote: > On Fri, Sep 04, 2026 at 01:37:00PM -0700, Bill Wendling wrote: > > 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 export namespacing should limit it to just KUnit, so that should be > okay, I would think? My guide for this in the past has been "did the > kernel get less fast/safe because of Kunit exports, etc?" I don't > _think_ that's the case here. > > So, I guess it's down to a matter of taste by USER_NS maintainers? I've > move Christian to "To:" from "Cc:", as I think he is the defacto USER_NS > maintainer? I don't know, though, there is no MAINTAINERS entry for it. > My understanding of Bradley's comment is that we could use visibility.h, but that doesn't give us much because we couldn't run these tests with CONFIG_KUNIT=m anyway. But as I said, I'm not a KUnit guru, which is why I'm leaving the decision to maintainers and reviewers. I'm fine with using the "visibility.h" if that's what people want, and it works.
> > > > 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... > > It is tricky to catch the traps sanely with KUnit. All the Oops tests > I've written for it are in LKDTM, and only __bdos is tested in > lib/tests/overflow_kunit.c. I was playing with catching Oops in KUnit > for testing KCFI recently, but haven't come back to finishing it safely: > https://lore.kernel.org/lkml/[email protected]/ > I was using the kunit "run/catch" functions, but it's not complete because they don't tell what underlying issue actually caused the trap, only that a trap occurred. So not ideal. Maybe the LKDTM is the best way to go then... -bw
