Re: [PATCH bpf v2 3/4] bpf: track subprog poke correctly

On Tue, Jun 22, 2021 at 4:02 PM John Fastabend [off-list ref] wrote:

Alexei Starovoitov wrote:

quoted

On Wed, Jun 16, 2021 at 03:55:39PM -0700, John Fastabend wrote:

quoted

-static void bpf_free_used_maps(struct bpf_prog_aux *aux)
+void bpf_free_used_maps(struct bpf_prog_aux *aux)
 {
    __bpf_free_used_maps(aux, aux->used_maps, aux->used_map_cnt);
    kfree(aux->used_maps);

@@ -2211,8 +2211,10 @@ static void bpf_prog_free_deferred(struct work_struct *work)
 #endif
    if (aux->dst_trampoline)
            bpf_trampoline_put(aux->dst_trampoline);
-   for (i = 0; i < aux->func_cnt; i++)
+   for (i = 0; i < aux->func_cnt; i++) {
+           bpf_free_used_maps(aux->func[i]->aux);
            bpf_jit_free(aux->func[i]);
+   }

The sub-progs don't have all the properties of the main prog.
Only main prog suppose to keep maps incremented.
After this patch the prog with 100 subprogs will artificially bump maps
refcnt 100 times as a workaround for poke_tab access.

Yep.

quoted

May be we can use single poke_tab in the main prog instead.
Looks like jit_subprogs is splitting the poke_tab into individual arrays
for each subprog, but maps are tracked by the main prog only.
That's the root cause of the issue, right?

Correct.

quoted

I think that split of poke_tab isn't necessary.
bpf_int_jit_compile() can look into main prog poke_tab instead.
Then the loop:
for (j = 0; j < prog->aux->size_poke_tab)
    bpf_jit_add_poke_descriptor(func[i], &prog->aux->poke_tab[j]);
can be removed (It will address the concern in patch 2 as well, right?)
And hopefully will fix UAF too?

Looks like it to me as well. A few details to work out around
imm value and emit hooks on the jit side, but looks doable to me.
I'll give it a try tomorrow or tonight.

Perfect. Thank you John.