Re: [PATCH v2 04/16] riscv: Add kprobes instruction simulation KUnit

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The KUnit test executes an uninterruptible 4.29 billion iteration loop, guaranteeing kernel soft lockups and breaking CI.
- [Medium] The KUnit test logic is tautological and its state initialization masks branch outcomes, halving test coverage.
--

commit 0b416648ab5f17f7e140c671781502c76b641895
Author: Charlie Jenkins <thecharlesjenkins@gmail.com>

riscv: Add kprobes instruction simulation KUnit

This commit introduces a KUnit test that iterates through all 32-bit
integers to validate kprobes instruction simulation. The tests are gated
behind a new CONFIG_RISCV_KPROBES_SIMULATE_KUNIT kconfig option.

diff --git a/arch/riscv/kernel/tests/Kconfig.debug b/arch/riscv/kernel/tests/Kconfig.debug
index 40f8dafffa0ab..9eda8938ec15b 100644
--- a/arch/riscv/kernel/tests/Kconfig.debug
+++ b/arch/riscv/kernel/tests/Kconfig.debug
@@ -42,6 +42,19 @@ config RISCV_KPROBES_KUNIT
 
          If unsure, say N.
 
> +config RISCV_KPROBES_SIMULATE_KUNIT
> +       tristate "KUnit test for riscv kprobes instruction simulation" if !KUNIT_ALL_TESTS
> +       depends on KUNIT
> +       depends on KPROBES
> +       default KUNIT_ALL_TESTS

[Severity: High]
Will this default KUNIT_ALL_TESTS cause CI pipelines to break?

Since the test iterates through all 32-bit integers, it appears it could
cause standard testing environments to timeout or lockup when they enable
all tests.

> +       help
> +         Enable testing for riscv kprobes instruction simulation. Useful for

[ ... ]

diff --git a/arch/riscv/kernel/tests/kprobes/test-kprobes-simulate.c b/arch/riscv/kernel/tests/kprobes/test-kprobes-simulate.c
new file mode 100644
index 0000000000000..d82706685823d
--- /dev/null
+++ b/arch/riscv/kernel/tests/kprobes/test-kprobes-simulate.c
@@ -0,0 +1,250 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include <linux/kernel.h>

[ ... ]

> +static void test_kprobe_simulate_riscv(struct kunit *test)
> +{
> +	unsigned int addr = 0xdeadbeef;
> +	unsigned int i = 0;
> +
> +	do {
> +		struct pt_regs regs = { 0 };

[Severity: High]
Is it safe to run a 4.29 billion iteration loop inside the kernel without
yielding the CPU?

Looking at test_kprobe_simulate_riscv(), there doesn't appear to be any
mechanism like cond_resched() inside the loop or the simulated functions.
Won't this monopolize the CPU and reliably trigger the soft lockup watchdog
or RCU stall detectors?

[Severity: Medium]
Does initializing the registers to zero on every iteration reduce the coverage
of conditional branches?

If source registers like rs1 and rs2 are never populated with non-zero values,
won't the branch conditions evaluate to a single static outcome? For example,
a beq (0 == 0) will always be true, and bne (0 != 0) will always be false.
This seems to skip testing the alternate branch paths.

> +
> +		if (riscv_insn_is_jal(i)) {

[ ... ]

> +		} else if (riscv_insn_is_auipc(i)) {
> +			s32 offset = riscv_insn_auipc_extract_imm(i);
> +			u32 rd_index = riscv_insn_auipc_extract_xd(i);
> +
> +			simulate_auipc(i, addr, &regs);
> +
> +			KUNIT_EXPECT_EQ_MSG(test, regs.epc, addr + 4,
> +					    "auipc instruction (0x%x) incorrectly simulated", i);
> +
> +			if (rd_index)
> +				KUNIT_EXPECT_EQ_MSG(
> +					test,
> +					riscv_insn_reg_get_val((unsigned long *)&regs, rd_index),
> +					(unsigned long)addr + offset,
> +					"auipc instruction (0x%x) incorrectly simulated", i);

[Severity: Medium]
Does extracting the expected values using the same macros as the implementation
make the test tautological?

If a macro like riscv_insn_auipc_extract_imm() is flawed, both the test and
the implementation will compute the identical incorrect value, causing the
test to spuriously pass.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260621-riscv_insn_table-v2-0-7f0810ee2274@gmail.com?part=4