Re: [PATCH AUTOSEL 5.4 08/10] selftests/bpf: Fix array access with signed variable test
On Sun, Dec 20, 2020 at 01:05:19PM +0100, Jean-Philippe Brucker wrote: Hi, On Sat, Dec 19, 2020 at 10:34:55PM -0500, Sasha Levin wrote: From: Jean-Philippe Brucker [ Upstream commit 77ce220c0549dcc3db8226c61c60e83fc59dfafc ] The test fails because of a recent fix to the verifier, even though this That fix is commit b02709587ea3 ("bpf: Fix propagation of 32-bit signed bounds from 64-bit bounds.") upstream, which only needed backport to 5.9. So although backporting this patch to 5.4 shouldn't break anything, I wouldn't bother. I'll drop it from 5.4, thanks! -- Thanks, Sasha
Re: [PATCH AUTOSEL 5.4 08/10] selftests/bpf: Fix array access with signed variable test
Hi, On Sat, Dec 19, 2020 at 10:34:55PM -0500, Sasha Levin wrote: > From: Jean-Philippe Brucker > > [ Upstream commit 77ce220c0549dcc3db8226c61c60e83fc59dfafc ] > > The test fails because of a recent fix to the verifier, even though this That fix is commit b02709587ea3 ("bpf: Fix propagation of 32-bit signed bounds from 64-bit bounds.") upstream, which only needed backport to 5.9. So although backporting this patch to 5.4 shouldn't break anything, I wouldn't bother. Thanks, Jean > program is valid. In details what happens is: > > 7: (61) r1 = *(u32 *)(r0 +0) > > Load a 32-bit value, with signed bounds [S32_MIN, S32_MAX]. The bounds > of the 64-bit value are [0, U32_MAX]... > > 8: (65) if r1 s> 0x goto pc+1 > > ... therefore this is always true (the operand is sign-extended). > > 10: (b4) w2 = 11 > 11: (6d) if r2 s> r1 goto pc+1 > > When true, the 64-bit bounds become [0, 10]. The 32-bit bounds are still > [S32_MIN, 10]. > > 13: (64) w1 <<= 2 > > Because this is a 32-bit operation, the verifier propagates the new > 32-bit bounds to the 64-bit ones, and the knowledge gained from insn 11 > is lost. > > 14: (0f) r0 += r1 > 15: (7a) *(u64 *)(r0 +0) = 4 > > Then the verifier considers r0 unbounded here, rejecting the test. To > make the test work, change insn 8 to check the sign of the 32-bit value. > > Signed-off-by: Jean-Philippe Brucker > Acked-by: John Fastabend > Signed-off-by: Alexei Starovoitov > Signed-off-by: Sasha Levin > --- > tools/testing/selftests/bpf/verifier/array_access.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/testing/selftests/bpf/verifier/array_access.c > b/tools/testing/selftests/bpf/verifier/array_access.c > index f3c33e128709b..a80d806ead15f 100644 > --- a/tools/testing/selftests/bpf/verifier/array_access.c > +++ b/tools/testing/selftests/bpf/verifier/array_access.c > @@ -68,7 +68,7 @@ > BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem), > BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 9), > BPF_LDX_MEM(BPF_W, BPF_REG_1, BPF_REG_0, 0), > - BPF_JMP_IMM(BPF_JSGT, BPF_REG_1, 0x, 1), > + BPF_JMP32_IMM(BPF_JSGT, BPF_REG_1, 0x, 1), > BPF_MOV32_IMM(BPF_REG_1, 0), > BPF_MOV32_IMM(BPF_REG_2, MAX_ENTRIES), > BPF_JMP_REG(BPF_JSGT, BPF_REG_2, BPF_REG_1, 1), > -- > 2.27.0 >
[PATCH AUTOSEL 5.4 08/10] selftests/bpf: Fix array access with signed variable test
From: Jean-Philippe Brucker [ Upstream commit 77ce220c0549dcc3db8226c61c60e83fc59dfafc ] The test fails because of a recent fix to the verifier, even though this program is valid. In details what happens is: 7: (61) r1 = *(u32 *)(r0 +0) Load a 32-bit value, with signed bounds [S32_MIN, S32_MAX]. The bounds of the 64-bit value are [0, U32_MAX]... 8: (65) if r1 s> 0x goto pc+1 ... therefore this is always true (the operand is sign-extended). 10: (b4) w2 = 11 11: (6d) if r2 s> r1 goto pc+1 When true, the 64-bit bounds become [0, 10]. The 32-bit bounds are still [S32_MIN, 10]. 13: (64) w1 <<= 2 Because this is a 32-bit operation, the verifier propagates the new 32-bit bounds to the 64-bit ones, and the knowledge gained from insn 11 is lost. 14: (0f) r0 += r1 15: (7a) *(u64 *)(r0 +0) = 4 Then the verifier considers r0 unbounded here, rejecting the test. To make the test work, change insn 8 to check the sign of the 32-bit value. Signed-off-by: Jean-Philippe Brucker Acked-by: John Fastabend Signed-off-by: Alexei Starovoitov Signed-off-by: Sasha Levin --- tools/testing/selftests/bpf/verifier/array_access.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/testing/selftests/bpf/verifier/array_access.c b/tools/testing/selftests/bpf/verifier/array_access.c index f3c33e128709b..a80d806ead15f 100644 --- a/tools/testing/selftests/bpf/verifier/array_access.c +++ b/tools/testing/selftests/bpf/verifier/array_access.c @@ -68,7 +68,7 @@ BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem), BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 9), BPF_LDX_MEM(BPF_W, BPF_REG_1, BPF_REG_0, 0), - BPF_JMP_IMM(BPF_JSGT, BPF_REG_1, 0x, 1), + BPF_JMP32_IMM(BPF_JSGT, BPF_REG_1, 0x, 1), BPF_MOV32_IMM(BPF_REG_1, 0), BPF_MOV32_IMM(BPF_REG_2, MAX_ENTRIES), BPF_JMP_REG(BPF_JSGT, BPF_REG_2, BPF_REG_1, 1), -- 2.27.0