Re: [PATCH v11 bpf-next 01/12] bpf, x86: add new map type: instructions array

[Anton Protopopov <a.s.protopopov@gmail.com>]

On 25/11/06 09:08AM, Alexei Starovoitov wrote:
> On Thu, Nov 6, 2025 at 1:54 AM Anton Protopopov
> <a.s.protopopov@gmail.com> wrote:
> >
> > On 25/11/05 06:03PM, Alexei Starovoitov wrote:
> > > On Wed, Nov 5, 2025 at 12:58 AM Anton Protopopov
> > > <a.s.protopopov@gmail.com> wrote:
> > > > @@ -21695,6 +21736,8 @@ static int jit_subprogs(struct bpf_verifier_env *env)
> > > >                 func[i]->aux->jited_linfo = prog->aux->jited_linfo;
> > > >                 func[i]->aux->linfo_idx = env->subprog_info[i].linfo_idx;
> > > >                 func[i]->aux->arena = prog->aux->arena;
> > > > +               func[i]->aux->used_maps = env->used_maps;
> > > > +               func[i]->aux->used_map_cnt = env->used_map_cnt;
> > >
> > > ...
> > >
> > > > It might be called before the used_maps are copied into aux...
> > >
> > > wat?
> >
> > It is called from fixup_call_arg() which happens before
> > the env->prog->aux->used_maps is populated as a copy of
> > env->used_maps.
> >
> > In any case, I will take a closer look and follow up on
> > this after Kubecon (which is the next week).
> 
> Pls look at the diff
> and also
> line 22074:
> func[i]->aux->main_prog_aux = prog->aux;
> line 22099:
> func[i]->aux->used_maps = env->used_maps;

[Sorry for the delay, I was travelling and didn't have access to my lab.]

I've seen this diff and tested it before sending the previous reply.
It didn't work, and it doesn't work now on bpf-next/master: the
"./test_progs -a bpf_insn_array/deletions-with-functions" test
still breaks.

The reason is as follows. There are two cases for which JIT is called
differently.

1) For a program without sub-functions the JIT is called from the
bpf_prog_select_runtime() function. By this time
aux->main_prog_aux->used_maps are populated and thus
aux->main_prog_aux could be used; or just aux, as there is only one
prog.

2) When program has sub-functions, say one, the jit is called from
jit_subprogs() and later the call in bpf_prog_select_runtime() is
skipped. The jit_subprogs() is called before the bpf_check()
epilogue, and thus not func[i]->aux nor aux->main_prog_aux
contain a copy of used_maps, it is only copied later.

To make two cases look the same ("aux->used_maps is correct"), I've
added

    func[i]->aux->used_maps = env->used_maps;
    func[i]->aux->used_map_cnt = env->used_map_cnt;

Note, again, that in case 2, without this copy, no functions will
have used_maps set, even main_prog.

> > > on top of the set:
> > > diff --git a/kernel/bpf/bpf_insn_array.c b/kernel/bpf/bpf_insn_array.c
> > > index 61ce52882632..97fcde6d7f07 100644
> > > --- a/kernel/bpf/bpf_insn_array.c
> > > +++ b/kernel/bpf/bpf_insn_array.c
> > > @@ -278,8 +278,8 @@ void bpf_prog_update_insn_ptrs(struct bpf_prog
> > > *prog, u32 *offsets, void *image)
> > >         if (!offsets || !image)
> > >                 return;
> > >
> > > -       for (i = 0; i < prog->aux->used_map_cnt; i++) {
> > > -               map = prog->aux->used_maps[i];
> > > +       for (i = 0; i < prog->aux->main_prog_aux->used_map_cnt; i++) {
> > > +               map = prog->aux->main_prog_aux->used_maps[i];
> > >                 if (!is_insn_array(map))
> > >                         continue;
> > >
> > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > index 1268fa075d4c..53b9a6cee156 100644
> > > --- a/kernel/bpf/verifier.c
> > > +++ b/kernel/bpf/verifier.c
> > > @@ -22096,8 +22096,6 @@ static int jit_subprogs(struct bpf_verifier_env *env)
> > >                 func[i]->aux->jited_linfo = prog->aux->jited_linfo;
> > >                 func[i]->aux->linfo_idx = env->subprog_info[i].linfo_idx;
> > >                 func[i]->aux->arena = prog->aux->arena;
> > > -               func[i]->aux->used_maps = env->used_maps;
> > > -               func[i]->aux->used_map_cnt = env->used_map_cnt;
> > >                 num_exentries = 0;
> > >                 insn = func[i]->insnsi;
> > >                 for (j = 0; j < func[i]->len; j++, insn++) {
> > >
> > >
> > > all tests still pass.
> > >
> > > If I'm not missing anything, please send a follow up.
> > >
> > > The plan is to split prog_aux into main and subprog,
> > > and subprog will be a fraction of main.
> > > Right now we copy more and more fields for no good reason.
> > > Let's avoid this.