Re: [PATCH stable 6.6.y v3 3/4] selftests/bpf: Tests for per-insn sync_linked_regs() precision tracking

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Low] Newly added multi-line comments violate the strict BPF subsystem comment style by placing text on the same line as the opening `/*`.
- [Low] A comment in `linked_regs_bpf_x_dst` incorrectly claims to force `r0` to be precise, but the corresponding code actually targets `r3`.
- [Low] A comment in `linked_regs_too_many_regs` incorrectly states that `r{0-4}` will be precise, missing `r5`.
--

commit 85ad29f2c85dcd0b0ee951c5fc1bc2711d79a7a5
Author: Eduard Zingerman <eddyz87@gmail.com>

selftests/bpf: Tests for per-insn sync_linked_regs() precision tracking

This commit adds test cases to verify precision tracking for scalars gaining
range because of sync_linked_regs(). It includes checks for when more than 6
registers might gain range, precision propagation, and adds a reproducer
for a precision tracking bug.

Link: https://lore.kernel.org/bpf/20240718202357.1746514-4-eddyz87@gmail.com

> diff --git a/tools/testing/selftests/bpf/progs/verifier_scalar_ids.c b/tools/testing/selftests/bpf/progs/verifier_scalar_ids.c
> index f70392bf696c6..2eb85eb3a06cc 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_scalar_ids.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_scalar_ids.c
> @@ -47,6 +47,72 @@ __naked void linked_regs_bpf_k(void)
>  	: __clobber_all);
>  }
>  
> +/* Registers r{0,1,2} share same ID when 'if r1 > ...' insn is processed,
> + * check that verifier marks r{1,2} as precise while backtracking
> + * 'if r1 > ...' with r0 already marked.
> + */

[Severity: Low]
This isn't a bug, but the BPF subsystem multi-line comment style requires
the opening /* to be on its own line, rather than starting text on the first
line.

> +SEC("socket")
> +__success __log_level(2)

[ ... ]

> +/* Registers r{0,1,2} share same ID when 'if r1 > r3' insn is processed,
> + * check that verifier marks r{0,1,2} as precise while backtracking
> + * 'if r1 > r3' with r3 already marked.
> + */

[Severity: Low]
This isn't a bug, but here's another instance where the multi-line comment
starts text on the opening line instead of placing the /* on its own line.

> +SEC("socket")
> +__success __log_level(2)
> +__flag(BPF_F_TEST_STATE_FREQ)
> +__msg("frame0: regs=r3 stack= before 5: (2d) if r1 > r3 goto pc+0")
> +__msg("frame0: parent state regs=r0,r1,r2,r3 stack=:")
> +__msg("frame0: regs=r0,r1,r2,r3 stack= before 4: (b7) r3 = 7")
> +__naked void linked_regs_bpf_x_dst(void)
> +{
> +	asm volatile (
> +	/* r0 = random number up to 0xff */
> +	"call %[bpf_ktime_get_ns];"
> +	"r0 &= 0xff;"
> +	/* tie r0.id == r1.id == r2.id */
> +	"r1 = r0;"
> +	"r2 = r0;"
> +	"r3 = 7;"
> +	"if r1 > r3 goto +0;"
> +	/* force r0 to be precise, this eventually marks r1 and r2 as
> +	 * precise as well because of shared IDs
> +	 */
> +	"r4 = r10;"
> +	"r4 += r3;"

[Severity: Low]
This isn't a bug, but should the comment here say "force r3 to be precise"
instead of "force r0"? The code actually targets r3, so this appears to be
a copy-paste error from linked_regs_bpf_x_src.

> +	"r0 = 0;"
> +	"exit;"
> +	:
> +	: __imm(bpf_ktime_get_ns)
> +	: __clobber_all);
> +}

[ ... ]

> +SEC("socket")
> +__success __log_level(2)
> +__flag(BPF_F_TEST_STATE_FREQ)
> +/* check thar r0 and r6 have different IDs after 'if',
> + * collect_linked_regs() can't tie more than 6 registers for a single insn.
> + */

[Severity: Low]
This isn't a bug, but just a quick note that this multi-line comment also
doesn't follow the preferred BPF style of having the opening /* on its own
line.

> +__msg("8: (25) if r0 > 0x7 goto pc+0         ; R0=scalar(id=1")
> +__msg("9: (bf) r6 = r6                       ; R6_w=scalar(id=2")
> +/* check that r{0-5} are marked precise after 'if' */
> +__msg("frame0: regs=r0 stack= before 8: (25) if r0 > 0x7 goto pc+0")
> +__msg("frame0: parent state regs=r0,r1,r2,r3,r4,r5 stack=:")
> +__naked void linked_regs_too_many_regs(void)
> +{
> +	asm volatile (
> +	/* r0 = random number up to 0xff */
> +	"call %[bpf_ktime_get_ns];"
> +	"r0 &= 0xff;"
> +	/* tie r{0-6} IDs */
> +	"r1 = r0;"
> +	"r2 = r0;"
> +	"r3 = r0;"
> +	"r4 = r0;"
> +	"r5 = r0;"
> +	"r6 = r0;"
> +	/* propagate range for r{0-6} */
> +	"if r0 > 7 goto +0;"
> +	/* make r6 appear in the log */
> +	"r6 = r6;"
> +	/* force r0 to be precise,
> +	 * this would cause r{0-4} to be precise because of shared IDs
> +	 */

[Severity: Low]
This isn't a bug, but could the comment mention r{0-5} will be precise instead
of r{0-4}? The __msg assertion properly verifies regs=r0,r1,r2,r3,r4,r5, so
the text might be accidentally omitting r5.

> +	"r7 = r10;"
> +	"r7 += r0;"
> +	"r0 = 0;"
> +	"exit;"
> +	:
> +	: __imm(bpf_ktime_get_ns)
> +	: __clobber_all);
> +}

[ ... ]

> +/* Check that mark_chain_precision() for one of the conditional jump
> + * operands does not trigger equal scalars precision propagation.
> + */

[Severity: Low]
This isn't a bug, but this multi-line comment starts text on the opening
line, similar to the ones above.

> +SEC("socket")
> +__success __log_level(2)
> +__msg("3: (25) if r1 > 0x100 goto pc+0")
> +__msg("frame0: regs=r1 stack= before 2: (bf) r1 = r0")
> +__naked void cjmp_no_linked_regs_trigger(void)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1781194510.git.jt26wzz@gmail.com?part=3