Re: [PATCH 3/3] rv/reactors: add KUnit tests for reactor_panic

[Wen Yang <wen.yang@linux.dev>]

On 6/21/26 07:30, XIAO WU wrote:
> Hi Wen,
> 
> I came across a Sashiko AI code review [1] that flagged a potential NULL
> pointer dereference in the `test_panic_register_unregister()` test case
> added by this patch (commit 8655782285e2).  The review's analysis seemed
> plausible, so I spun up a QEMU environment to see whether it could be
> reproduced in practice.
> 
> The short version: yes, it triggers a real kernel BUG + Oops.  See below
> for the crash log and the reproduction approach.
> 
> On Tue, 16 Jun 2026 at 00:44, Wen Yang wrote:
>  > Add KUnit tests for the panic reactor covering:
>  > - Reactor registration and unregistration lifecycle
>  > - Panic notifier chain reachability
> ...
>  > +static void test_panic_register_unregister(struct kunit *test)
>  > +{
>  > +    int ret;
>  > +
>  > +    ret = rv_register_reactor(&mock_panic_reactor);
>  > +    KUNIT_EXPECT_EQ(test, ret, 0);
>  > +    KUNIT_EXPECT_STREQ(test, mock_panic_reactor.name, "test_panic");
>  > +
>  > +    rv_unregister_reactor(&mock_panic_reactor);
> 
> This is the function the review highlighted.  The issue is:
> 
>    - `KUNIT_EXPECT_EQ()` does *not* abort the test on failure.
>    - If `rv_register_reactor()` fails (e.g. because another reactor
>      named "test_panic" was already registered), the .list node of the
>      statically-allocated `mock_panic_reactor` is never added to any
>      list — it remains zero-initialized (prev = NULL, next = NULL).
>    - `rv_unregister_reactor()` then unconditionally calls `list_del()`
>      on this uninitialized list_head, which hits the NULL pointers.
> 
> I was able to reproduce this reliably.  The trigger condition is
> surprisingly simple: if any code path registers a reactor named
> "test_panic" before the KUnit suite runs, the test crashes the kernel.
> 
> [Reproduction approach]
> 
> I rebuilt the kernel with a small late_initcall in rv_reactors.c that
> pre-registers "test_panic" (simulating what would happen if, say, a
> kernel module or another subsystem registered a reactor with the same
> name before the KUnit tests execute):
> 
>    static int __init prereg_test_panic(void)
>    {
>        static struct rv_reactor prereg = {
>            .name = "test_panic",
>            .description = "pre-registered to simulate name collision",
>        };
>        return rv_register_reactor(&prereg);
>    }
>    late_initcall(prereg_test_panic);
> 
> The KUnit tests then auto-run at boot (kunit_run_all_tests).  The
> test_panic_register_unregister case fails registration with -EINVAL due
> to the duplicate name, the KUNIT_EXPECT_EQ does not abort, and
> rv_unregister_reactor() crashes on the uninitialized list.
> 
> [Crash log — kernel 7.1.0-next-20260615, CONFIG_DEBUG_LIST=y]
> 
>    Reactor test_panic is already registered
>        # test_panic_register_unregister: EXPECTATION FAILED at 
> kernel/trace/rv/reactor_panic_kunit.c:68
>        Expected ret == 0, but
>            ret == -22 (0xffffffffffffffea)
>    list_del corruption, ffffffff8ecce2f8->next is NULL
>    ------------[ cut here ]------------
>    kernel BUG at lib/list_debug.c:52!
>    Oops: invalid opcode: 0000 [#1] SMP KASAN NOPTI
>    CPU: 1 UID: 0 PID: 5028 Comm: kunit_try_catch Tainted: G   N
>    RIP: 0010:__list_del_entry_valid_or_report+0xf2/0x200
>    Call Trace:
>     <TASK>
>     rv_unregister_reactor+0x37/0x190
>     test_panic_register_unregister+0x1de/0x2e0
>     kunit_try_run_case+0x1d2/0x520
>     kunit_generic_run_threadfn_adapter+0x89/0x100
>     kthread+0x387/0x4a0
>     ret_from_fork+0xb2c/0xdd0
>     </TASK>
>    Kernel panic - not syncing: Fatal exception
> 
> The crash is in `rv_unregister_reactor()`, called from
> `test_panic_register_unregister()`.  The `list_del()` in
> `rv_unregister_reactor()` has no guard against a list node that was
> never added to any list.  With CONFIG_DEBUG_LIST=y the corruption is
> caught explicitly; without it this would be a silent NULL dereference.
> 
> [Suggested fix]
> 
> The most straightforward fix is to use `KUNIT_ASSERT_EQ()` instead of
> `KUNIT_EXPECT_EQ()` for the registration result, so the test aborts
> before reaching `rv_unregister_reactor()` on a failed registration:
> 
>    static void test_panic_register_unregister(struct kunit *test)
>    {
>        int ret;
> 
>        ret = rv_register_reactor(&mock_panic_reactor);
> -     KUNIT_EXPECT_EQ(test, ret, 0);
> +     KUNIT_ASSERT_EQ(test, ret, 0);
>        KUNIT_EXPECT_STREQ(test, mock_panic_reactor.name, "test_panic");
> 
>        rv_unregister_reactor(&mock_panic_reactor);
>    }
> 
> An alternative (or complementary) approach would be to add a guard in
> `rv_unregister_reactor()` itself — e.g. checking whether the reactor is
> actually on the list before calling `list_del()`.  That would make the
> API more robust against future callers making the same mistake.
> 
> The same pattern likely applies to the printk reactor tests in patch
> 2/3, though I haven't tested those.
> 

Okay, thank you.
We've noted this is related to a Kunit test. We'll incorporate the 
improvement in the v2.
Thanks again.

--
Wen


> Full PoC code follows.
> 
> [PoC part 1 — Kernel-space: late_initcall to create the name collision]
> 
> This is what was added to kernel/trace/rv/rv_reactors.c (or could be
> built as a standalone kernel module — see preregister.c below).  It
> pre-registers "test_panic" before KUnit auto-runs, so the test's own
> rv_register_reactor() fails with -EINVAL:
> 
>    static int __init prereg_test_panic(void)
>    {
>        static struct rv_reactor prereg = {
>            .name = "test_panic",
>            .description = "pre-registered to simulate name collision",
>        };
>        return rv_register_reactor(&prereg);
>    }
>    late_initcall(prereg_test_panic);
> 
> [PoC part 2 — Userspace: trigger the KUnit test via debugfs]
> 
> poc.c:
> ---8<----------------------------------------------------------------
> /*
>   * POC: NULL pointer dereference in rv_unregister_reactor()
>   *
>   * Bug location: kernel/trace/rv/reactor_panic_kunit.c
>   *   test_panic_register_unregister()
>   *
>   * Bug: When rv_register_reactor() fails (because "test_panic" is already
>   * registered), the test calls rv_unregister_reactor() unconditionally.
>   * This performs list_del() on a zero-initialized list_head (never added
>   * to any list), causing a NULL pointer dereference crash.
>   *
>   * Trigger: With a kernel that has pre-registered "test_panic" reactor,
>   * simply trigger the KUnit test via debugfs "run" file. The test's
>   * rv_register_reactor() fails with -EINVAL (duplicate), and the 
> subsequent
>   * rv_unregister_reactor() crashes on the uninitialized list.
>   */
> 
> #define _GNU_SOURCE
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> #include <unistd.h>
> #include <sys/types.h>
> #include <sys/stat.h>
> #include <fcntl.h>
> #include <errno.h>
> 
> #define KUNIT_RUN_PATH "/sys/kernel/debug/kunit/rv_reactor_panic/run"
> 
> int main(int argc, char **argv)
> {
>      int fd, ret;
> 
>      setbuf(stdout, NULL);
> 
>      printf("[+] POC: Triggering NULL deref in rv_unregister_reactor\n");
>      printf("[+] Target: %s\n\n", KUNIT_RUN_PATH);
> 
>      /* Mount debugfs if needed */
>      if (access("/sys/kernel/debug", F_OK) != 0) {
>          printf("[*] Mounting debugfs...\n");
>          ret = system("mount -t debugfs none /sys/kernel/debug/ 
> 2>/dev/null");
>          (void)ret;
>      }
> 
>      /* Verify KUnit path exists */
>      if (access(KUNIT_RUN_PATH, W_OK) != 0) {
>          printf("[-] Cannot access %s: %m\n", KUNIT_RUN_PATH);
>          printf("[*] Available KUnit suites:\n");
>          fflush(stdout);
>          system("ls -la /sys/kernel/debug/kunit/ 2>&1");
>          return 1;
>      }
> 
>      printf("[*] Test_panic reactor should be pre-registered at boot\n");
>      printf("[*] Triggering KUnit test suite...\n\n");
> 
>      /*
>       * Write to the KUnit run file. This executes
>       * __kunit_test_suites_init() -> kunit_run_tests() which
>       * runs the reactor_panic_kunit test cases including
>       * test_panic_register_unregister.
>       *
>       * With "test_panic" pre-registered:
>       * 1. rv_register_reactor() returns -EINVAL (duplicate)
>       * 2. KUNIT_EXPECT_EQ doesn't abort
>       * 3. rv_unregister_reactor() calls list_del() on NULL list
>       * 4. BOOM: list corruption / NULL deref / kernel crash
>       */
>      fd = open(KUNIT_RUN_PATH, O_WRONLY);
>      if (fd < 0) {
>          printf("[-] open failed: %m\n");
>          return 1;
>      }
> 
>      printf("[!] Writing to %s - triggering the crash now...\n", 
> KUNIT_RUN_PATH);
>      fflush(stdout);
> 
>      ret = write(fd, "1", 1);
>      if (ret < 0) {
>          printf("[-] write failed: %m\n");
>      } else {
>          printf("[+] Write succeeded (ret=%d)\n", ret);
>      }
> 
>      close(fd);
> 
>      /*
>       * If we reach here without crashing, let the user know
>       */
>      printf("\n[*] If the system is still alive, check dmesg:\n");
>      printf("    dmesg | grep -i -E 
> 'list_del|list_add|list_corrupt|NULL|BUG|oops\n");
>      printf("\n[*] dmesg output:\n");
>      fflush(stdout);
>      system("dmesg | tail -60");
> 
>      printf("\n[+] POC completed.\n");
> 
>      return 0;
> }
> ---8<----------------------------------------------------------------
> 
> Compile with:
>    gcc -o poc poc.c -static
> 
> [PoC part 3 — Kernel module alternative (standalone)]
> 
> If you prefer not to modify rv_reactors.c directly, the same name
> collision can be created by loading this module before running the
> KUnit test.  Note: this requires rv_register_reactor() to be exported
> (or resolved via kallsyms), which it may not be in the current tree.
> In that case the late_initcall approach above is the way to go.
> 
> preregister.c:
> ---8<----------------------------------------------------------------
> #include <linux/module.h>
> #include <linux/kernel.h>
> #include <linux/rv.h>
> 
> static struct rv_reactor prereg_reactor = {
>      .name = "test_panic",
>      .description = "pre-registered to trigger KUnit bug",
> };
> 
> static int __init prereg_init(void)
> {
>      int ret;
>      ret = rv_register_reactor(&prereg_reactor);
>      if (ret < 0) {
>          pr_err("preregister: rv_register_reactor failed: %d\n", ret);
>          return ret;
>      }
>      pr_info("preregister: registered 'test_panic' reactor\n");
>      return 0;
> }
> 
> static void __exit prereg_exit(void)
> {
>      rv_unregister_reactor(&prereg_reactor);
>      pr_info("preregister: unregistered 'test_panic' reactor\n");
> }
> 
> module_init(prereg_init);
> module_exit(prereg_exit);
> MODULE_LICENSE("GPL");
> ---8<----------------------------------------------------------------
> 
> 
> [1] 
> https://sashiko.dev/#/patchset/cover.1781541556.git.wen.yang%40linux.dev
>      (Sashiko AI code review — "Null Pointer Dereference", Severity: High)
> 
> Thanks,
> XIAO
>