Re: [PATCH bpf-next 3/3] selftests/bpf: Disassembler tests for verifier.c:convert_ctx_access()

[Eduard Zingerman <eddyz87@gmail.com>]

On Fri, 2023-03-03 at 13:35 -0800, Alexei Starovoitov wrote:
> On Fri, Mar 3, 2023 at 1:24 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > 
> > On Fri, 2023-03-03 at 12:28 -0800, Alexei Starovoitov wrote:
> > > On Fri, Mar 03, 2023 at 12:55:07AM +0200, Eduard Zingerman wrote:
> > > > Function verifier.c:convert_ctx_access() applies some rewrites to BPF
> > > > instructions that read or write BPF program context. This commit adds
> > > > machinery to allow test cases that inspect BPF program after these
> > > > rewrites are applied.
> > > > 
> > > > An example of a test case:
> > > > 
> > > >   {
> > > >         // Shorthand for field offset and size specification
> > > >     N(CGROUP_SOCKOPT, struct bpf_sockopt, retval),
> > > > 
> > > >         // Pattern generated for field read
> > > >     .read  = "$dst = *(u64 *)($ctx + bpf_sockopt_kern::current_task);"
> > > >              "$dst = *(u64 *)($dst + task_struct::bpf_ctx);"
> > > >              "$dst = *(u32 *)($dst + bpf_cg_run_ctx::retval);",
> > > > 
> > > >         // Pattern generated for field write
> > > >     .write = "*(u64 *)($ctx + bpf_sockopt_kern::tmp_reg) = r9;"
> > > >              "r9 = *(u64 *)($ctx + bpf_sockopt_kern::current_task);"
> > > >              "r9 = *(u64 *)(r9 + task_struct::bpf_ctx);"
> > > >              "*(u32 *)(r9 + bpf_cg_run_ctx::retval) = $src;"
> > > >              "r9 = *(u64 *)($ctx + bpf_sockopt_kern::tmp_reg);" ,
> > > >   },
> > > > 
> > > > For each test case, up to three programs are created:
> > > > - One that uses BPF_LDX_MEM to read the context field.
> > > > - One that uses BPF_STX_MEM to write to the context field.
> > > > - One that uses BPF_ST_MEM to write to the context field.
> > > > 
> > > > The disassembly of each program is compared with the pattern specified
> > > > in the test case.
> > > > 
> > > > Kernel code for disassembly is reused (as is in the bpftool).
> > > > To keep Makefile changes to the minimum, symbolic links to
> > > > `kernel/bpf/disasm.c` and `kernel/bpf/disasm.h ` are added.
> > > ...
> > > > +static regex_t *compile_regex(char *pat)
> > > > +{
> > > > +   regex_t *re;
> > > > +   int err;
> > > > +
> > > > +   re = malloc(sizeof(regex_t));
> > > > +   if (!re) {
> > > > +           PRINT_FAIL("Can't alloc regex\n");
> > > > +           return NULL;
> > > > +   }
> > > > +
> > > > +   err = regcomp(re, pat, REG_EXTENDED);
> > > 
> > > Fancy.
> > 
> > In a good or in a bad way?
> > It is the shortest form I came up with...
> > 
> > > What is the cost of running this in test_progs?
> > > How many seconds does it add to run time?
> > 
> > About 0.13sec (including modprobe and process initialization):
> > 
> >   # time ./test_progs -a "ctx_rewrite/*"
> >   #58/1    ctx_rewrite/SCHED_CLS.tstamp:OK
> >   ...
> >   #58/20   ctx_rewrite/CGROUP_SOCKOPT.optval_end:OK
> >   #58      ctx_rewrite:OK
> >   Summary: 1/20 PASSED, 0 SKIPPED, 0 FAILED
> > 
> >   real  0m0.131s
> >   user  0m0.027s
> >   sys   0m0.046s
> > 
> > It loads 52 programs.
> 
> That's fine then. I was worried that compiling regex in a loop
> might be slow.

Oh... Regexes are compiled only once at test entry (in test_ctx_rewrite()),
sub-tests do not re-compile.