Re: [PATCH bpf-next 2/2] selftests/bpf: Add verifier test exercising jit PROBE_MEM logic

[Dave Marchevsky <davemarchevsky@meta.com>]

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@fb.com>
> 
> 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
> [...]