On 12/15/22 10:34 PM, Yonghong Song wrote:
>
>
> On 12/13/22 10:27 AM, Dave Marchevsky wrote:
>> This patch adds a test exercising logic that was fixed / improved in
>> the previous patch in the series, as well as general sanity checking for
>> jit's PROBE_MEM logic which should've been unaffected by the previous
>> patch.
>>
>> The added verifier test does the following:
>> * Acquire a referenced kptr to struct prog_test_ref_kfunc using
>> existing net/bpf/test_run.c kfunc
>> * Helper returns ptr to a specific prog_test_ref_kfunc whose first
>> two fields - both ints - have been prepopulated w/ vals 42 and
>> 108, respectively
>> * kptr_xchg the acquired ptr into an arraymap
>> * Do a direct map_value load of the just-added ptr
>> * Goal of all this setup is to get an unreferenced kptr pointing to
>> struct with ints of known value, which is the result of this step
>> * Using unreferenced kptr obtained in previous step, do loads of
>> prog_test_ref_kfunc.a (offset 0) and .b (offset 4)
>> * Then incr the kptr by 8 and load prog_test_ref_kfunc.a again (this
>> time at offset -8)
>> * Add all the loaded ints together and return
>>
>> Before the PROBE_MEM fixes in previous patch, the loads at offset 0 and
>> 4 would succeed, while the load at offset -8 would incorrectly fail
>> runtime check emitted by the JIT and 0 out dst reg as a result. This
>> confirmed by retval of 150 for this test before previous patch - since
>> second .a read is 0'd out - and a retval of 192 with the fixed logic.
>>
>> The test exercises the two optimizations to fixed logic added in last
>> patch as well:
>> * BPF_LDX_MEM(BPF_W, BPF_REG_1, BPF_REG_0, 0) exercises "insn->off is
>> 0, no need to add / sub from src_reg" optimization
>> * BPF_LDX_MEM(BPF_W, BPF_REG_0, BPF_REG_0, -8) exercises "src_reg ==
>> dst_reg, no need to restore src_reg after load" optimization
>>
>> Signed-off-by: Dave Marchevsky <davemarchevsky@xxxxxx>
>
> The test is quite complicated. Is it possible we could write a C code
> with every small portion of asm to test jit functionality. For
> b = p->a, the asm will simulate like below
> p += offsetof(p_type, a)
> b = *(u32 *)(p - offsetof(p_type, a))
> Could the above be a little bit simpler and easy to understand?
> I think you might be able to piggy back with some existing selftests.
>
Good point. Will give it a try.
>> ---
>> tools/testing/selftests/bpf/test_verifier.c | 75 ++++++++++++++----
>> tools/testing/selftests/bpf/verifier/jit.c | 84 +++++++++++++++++++++
>> 2 files changed, 146 insertions(+), 13 deletions(-)
>>
>> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
>> index 8c808551dfd7..14f8d0231e3c 100644
>> --- a/tools/testing/selftests/bpf/test_verifier.c
>> +++ b/tools/testing/selftests/bpf/test_verifier.c
>> @@ -55,7 +55,7 @@
>> #define MAX_UNEXPECTED_INSNS 32
>> #define MAX_TEST_INSNS 1000000
>> #define MAX_FIXUPS 8
>> -#define MAX_NR_MAPS 23
>> +#define MAX_NR_MAPS 24
>> #define MAX_TEST_RUNS 8
>> #define POINTER_VALUE 0xcafe4all
>> #define TEST_DATA_LEN 64
>> @@ -131,6 +131,7 @@ struct bpf_test {
>> int fixup_map_ringbuf[MAX_FIXUPS];
>> int fixup_map_timer[MAX_FIXUPS];
>> int fixup_map_kptr[MAX_FIXUPS];
>> + int fixup_map_probe_mem_read[MAX_FIXUPS];
>> struct kfunc_btf_id_pair fixup_kfunc_btf_id[MAX_FIXUPS];
>> /* Expected verifier log output for result REJECT or VERBOSE_ACCEPT.
>> * Can be a tab-separated sequence of expected strings. An empty string
> [...]
