Re: [PATCH bpf-next v2 2/2] selftests/bpf: Add tests for string kfuncs

[Viktor Malik <vmalik@redhat.com>]

On 9/27/24 03:57, Eduard Zingerman wrote:
> On Thu, 2024-09-26 at 09:29 +0200, Viktor Malik wrote:
>> The tests attach to `raw_tp/bpf_testmod_test_write_bare` triggerred by
>> `trigger_module_test_write` which writes the string "aaa..." of the
>> given size.
>>
>> Signed-off-by: Viktor Malik <vmalik@redhat.com>
>> ---
> 
> I thought about making these tests more terse as follows:
> 
> --- 8< ----------------------------------
> 
> // SPDX-License-Identifier: GPL-2.0
> 
> #include <linux/bpf.h>
> #include <bpf/bpf_helpers.h>
> #include "bpf_misc.h"
> 
> int bpf_strcmp(const char *cs, const char *ct) __ksym;
> char *bpf_strchr(const char *s, int c) __ksym;
> 
> static char *abc = "abc";
> 
> #define __test(retval) SEC("raw_tp") __success __retval(retval)
> 
> __test(2) int test_strcmp(void *ctx) { return bpf_strcmp(abc, "abd"); }
> __test(1) int test_strchr(void *ctx) { return bpf_strchr(abc, 'b') - abc; }
> 
> char _license[] SEC("license") = "GPL";
> 
> ---------------------------------- >8 ---
> 
> (plus registration in tools/testing/selftests/bpf/prog_tests/verifier.c)
> 
> However, this does not pass verification with the following error:
> 
>     VERIFIER LOG:
>     =============
>     arg#0 reference type('UNKNOWN ') size cannot be determined: -22
>     0: R1=ctx() R10=fp0
>     ; __test(2) int test_strcmp(void *ctx) { return bpf_strcmp(abc, "abd"); } @ verifier_str.c:15
>     0: (18) r1 = 0xffff8881019533dc       ; R1_w=map_value(map=.rodata.str1.1,ks=4,vs=8,off=4)
>     2: (18) r2 = 0xffff8881019533d8       ; R2_w=map_value(map=.rodata.str1.1,ks=4,vs=8)
>     4: (85) call bpf_strcmp#64714
>     write into map forbidden, value_size=8 off=4 size=1
>     processed 3 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
>     =============
>     #503/1   verifier_str/test_strcmp:FAIL
> 
> Note that each string literal in the BPF program is in fact a pointer
> to a read-only map. Hence in current form these new functions are not
> very ergonomic. I think verifier should be extended to check 'const'
> qualifiers for the kfuncs and allowing access in such cases.

Yeah, I noticed the same problem when I was trying to come with shorter
tests. Teaching verifier to allow passing pointers to read-only maps to
these kfuncs is certainly a good thing, I'll have a look into it.

On the other hand, bpftrace stores string literals on stack so these
kfuncs would be useful to us straight away.

Viktor

> 
> [...]
>