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

[John Fastabend <john.fastabend@gmail.com>]

Alexei Starovoitov wrote:
> On Wed, Jun 16, 2021 at 03:55:39PM -0700, John Fastabend wrote:
> >  
> > -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.

> 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.

> 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.

Thanks,
John