Re: [PATCH v3 04/17] riscv: Add kprobes instruction simulation KUnit

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The test performs 4.29 billion iterations without yielding and is forcefully enabled by `KUNIT_ALL_TESTS`, causing CI hangs and softlockup panics.
- [Medium] The test zeroes the simulated register state before every instruction, heavily limiting conditional branch simulation coverage.
- [Low] The test calls multiple undefined functions for instruction simulation and register manipulation, causing a build failure.
- [Low] The `CONFIG_RISCV_KPROBES_SIMULATE_KUNIT` option is a tristate, but the KUnit test calls unexported core kernel symbols, causing a module build failure.
--

--- Patch [4]: [PATCH v3 04/17] riscv: Add kprobes instruction simulation KUnit ---
commit fd54e10f3ed9775912526763a87c12f6ae0730fd
Author: Charlie Jenkins <thecharlesjenkins@gmail.com>

riscv: Add kprobes instruction simulation KUnit

This commit adds a KUnit test for RISC-V kprobes instruction simulation.
It iterates through all 32-bit integers to validate simulation behavior.
The test is gated behind a new configuration 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

[Severity: Low]
Should this be a bool rather than a tristate? Building this as a module
results in a regression because the test calls unexported kernel symbols
like simulate_jal and simulate_jalr, causing linker errors.

> +       depends on KUNIT
> +       depends on KPROBES
> +       default KUNIT_ALL_TESTS

[Severity: High]
Does hiding the prompt and defaulting to KUNIT_ALL_TESTS force CI pipelines
to run this test? Because the test is exceptionally slow, this setup
leaves automated testing systems no way to disable it when all tests are
requested.

> +       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 @@

[ ... ]

> +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]
Will this loop execute 4.29 billion times without yielding the CPU? 
Without a call to cond_resched(), iterating through all 32-bit integers
causes a regression by triggering softlockup watchdogs and RCU stall panics.

[Severity: Medium]
Because regs is zero-initialized on every iteration, are the source
registers evaluated by branch instructions restricted to zero? This
regression limits the conditional branch simulation to static outcomes
like 0 == 0 and 0 < 0, entirely missing alternate execution paths.

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

[ ... ]

> +		if (riscv_insn_is_jalr(i)) {
> +			unsigned long reg_addr = 0xffff;
> +			s32 offset = riscv_insn_jalr_extract_imm(i);
> +			u32 rd_index = riscv_insn_jalr_extract_xd(i);
> +			u32 rs1_index = riscv_insn_jalr_extract_xs1(i);
> +
> +			if (rs1_index)
> +				riscv_insn_reg_set_val((unsigned long *)&regs, rs1_index, reg_addr);

[Severity: Low]
Can this compile successfully? It appears riscv_insn_reg_set_val is an
undefined function, whereas the kernel actually uses rv_insn_reg_set_val.

> +			else
> +				reg_addr = 0;

[ ... ]

> +		} else if (riscv_insn_is_beq(i)) {
> +			s32 offset = riscv_insn_beq_extract_imm(i);
> +			u32 rs1_index = riscv_insn_beq_extract_xs1(i);
> +			u32 rs2_index = riscv_insn_beq_extract_xs2(i);
> +
> +			simulate_beq(i, addr, &regs);

[Severity: Low]
Are these individual branch simulation functions like simulate_beq defined
anywhere? Calling these undefined functions causes a build regression, as
the kernel uses a unified simulate_branch instead.

> +
> +			if (riscv_insn_reg_get_val((unsigned long *)&regs, rs1_index) ==

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707-riscv_insn_table-v3-0-3f8e5f75ba97@gmail.com?part=4