On 7/15/24 5:44 PM, Eduard Zingerman wrote:
On Fri, 2024-07-12 at 16:44 -0700, Yonghong Song wrote:
Note: it feels like these test cases should be a part of the
reg_bounds.c:test_reg_bounds_crafted(), e.g.:
diff --git a/tools/testing/selftests/bpf/prog_tests/reg_bounds.c b/tools/testing/selftests/bpf/prog_tests/reg_bounds.c
index eb74363f9f70..4918414f8e36 100644
--- a/tools/testing/selftests/bpf/prog_tests/reg_bounds.c
+++ b/tools/testing/selftests/bpf/prog_tests/reg_bounds.c
@@ -2108,6 +2108,7 @@ static struct subtest_case crafted_cases[] = {
{S32, U32, {(u32)S32_MIN, 0}, {0, 0}},
{S32, U32, {(u32)S32_MIN, 0}, {(u32)S32_MIN, (u32)S32_MIN}},
{S32, U32, {(u32)S32_MIN, S32_MAX}, {S32_MAX, S32_MAX}},
+ {S64, U32, {0x0, 0x1f}, {0xffffffff80000000ULL, 0x00000000ffffffffULL}},
};
/* Go over crafted hard-coded cases. This is fast, so we do it as part of
Which produces the following log:
VERIFIER LOG:
========================
...
21: (ae) if w6 < w7 goto pc+3 ; R6=scalar(id=1,smin=0xffffffff80000000,smax=0xffffffff)
R7=scalar(id=2,smin=smin32=0,smax=umax=smax32=umax32=31,var_off=(0x0; 0x1f))
...
from 21 to 25: ... R6=scalar(id=1,smin=smin32=0,smax=umax=smax32=umax32=30,var_off=(0x0; 0x1f))
R7=scalar(id=2,smin=umin=smin32=umin32=1,smax=umax=smax32=umax32=31,var_off=(0x0; 0x1f)
25: ... R6=scalar(id=1,smin=smin32=0,smax=umax=smax32=umax32=30,var_off=(0x0; 0x1f))
R7=scalar(id=2,smin=umin=smin32=umin32=1,smax=umax=smax32=umax32=31,var_off=(0x0; 0x1f))
...
However, this would require adjustments to reg_bounds.c logic to
include this new range rule.
I added some logic in reg_bounds.c.
diff --git a/tools/testing/selftests/bpf/prog_tests/reg_bounds.c b/tools/testing/selftests/bpf/prog_tests/reg_bounds.c
index eb74363f9f70..c88602908cfe 100644
--- a/tools/testing/selftests/bpf/prog_tests/reg_bounds.c
+++ b/tools/testing/selftests/bpf/prog_tests/reg_bounds.c
@@ -441,6 +441,22 @@ static struct range range_refine(enum num_t x_t, struct range x, enum num_t y_t,
if (t_is_32(y_t) && !t_is_32(x_t)) {
struct range x_swap;
+ /* If we know that
+ * - *x* is in the range of signed 32bit value
+ * - *y_cast* range is 32-bit sign non-negative, and
+ * then *x* range can be narrowed to the interaction of
+ * *x* and *y_cast*. Otherwise, if the new range for *x*
+ * allows upper 32-bit 0xffffffff then the eventual new
+ * range for *x* will be out of signed 32-bit range
+ * which violates the origin *x* range.
+ */
+ if (x_t == S64 && y_t == S32 &&
+ !(y_cast.a & 0xffffffff80000000ULL) && !(y_cast.b & 0xffffffff80000000) &&
+ (long long)x.a >= S32_MIN && (long long)x.b <= S32_MAX) {
+ return range(S64, max_t(S64, y_cast.a, x.a),
+ min_t(S64, y_cast.b, x.b));
+ }
+
/* some combinations of upper 32 bits and sign bit can lead to
* invalid ranges, in such cases it's easier to detect them
* after cast/swap than try to enumerate all the conditions
@@ -2108,6 +2124,9 @@ static struct subtest_case crafted_cases[] = {
{S32, U32, {(u32)S32_MIN, 0}, {0, 0}},
{S32, U32, {(u32)S32_MIN, 0}, {(u32)S32_MIN, (u32)S32_MIN}},
{S32, U32, {(u32)S32_MIN, S32_MAX}, {S32_MAX, S32_MAX}},
+ {S64, U32, {0x0, 0x1f}, {0xffffffff80000000ULL, 0x000000007fffffffULL}},
+ {S64, U32, {0x0, 0x1f}, {0xffffffffffff8000ULL, 0x0000000000007fffULL}},
+ {S64, U32, {0x0, 0x1f}, {0xffffffffffffff80ULL, 0x000000000000007fULL}},
};
/* Go over crafted hard-coded cases. This is fast, so we do it as part of
The logic is very similar to kernel implementation but has a difference in generating
the final range. In reg_bounds implementation, the range is narrowed by intersecting
y_cast and x range which are necessary.
In kernel implementation, there is no interection since we only have one register
and two register has been compared before.
Eduard, could you take a look at the above code?
[...]
+SEC("socket")
+__description("LDSX, S8, subreg compare")
^^^ ^^^
Nit: test_progs parsing logic for -t option is too simplistic to
understand comas inside description, for example here is happens
after the command below:
# ./test_progs-cpuv4 -t "verifier_ldsx/LDSX, S8, subreg compare"
#455/1 verifier_ldsx/LDSX, S8:OK
#455/2 verifier_ldsx/LDSX, S8 @unpriv:OK
#455/3 verifier_ldsx/LDSX, S16:OK
#455/4 verifier_ldsx/LDSX, S16 @unpriv:OK
#455/5 verifier_ldsx/LDSX, S32:OK
...
As far as I understand, this happens because test_progs tries to
match words LDSX, S8 and "subreg compare" separately.
This does not happen when comas are not used in the description
(or if description is omitted in favor of the C function name of the test
(it is not possible to do -t verifier_ldsx/ldsx_s8_subreg_compare
if __description is provided)).
There are around 10 (or more) verifier_*.c files which has ',' in their
description. So I think we should add ',' support in test_progs infrastructure
w.r.t. description tag. I think this can be a follow up.
+__success __success_unpriv
+__naked void ldsx_s8_subreg_compare(void)
+{
+ asm volatile (
+ "call %[bpf_get_prandom_u32];"
+ "*(u64 *)(r10 - 8) = r0;"
+ "w6 = w0;"
+ "if w6 > 0x1f goto l0_%=;"
+ "r7 = *(s8 *)(r10 - 8);"
+ "if w7 > w6 goto l0_%=;"
+ "r1 = 0;"
+ "*(u64 *)(r10 - 8) = r1;"
+ "r2 = r10;"
+ "r2 += -8;"
+ "r1 = %[map_hash_48b] ll;"
+ "call %[bpf_map_lookup_elem];"
+ "if r0 == 0 goto l0_%=;"
+ "r0 += r7;"
+ "*(u64 *)(r0 + 0) = 1;"
+"l0_%=:"
+ "r0 = 0;"
+ "exit;"
+ :
+ : __imm(bpf_get_prandom_u32),
+ __imm_addr(map_hash_48b),
+ __imm(bpf_map_lookup_elem)
+ : __clobber_all);
+}
[...]
