[bpf,v2,2/4] bpf: map_poke_descriptor is being called with an unstable poke_tab[]

Message ID 162388411986.151936.3914295553899556046.stgit@john-XPS-13-9370
State New
Headers show
Series
  • BPF fixes mixed tail and bpf2bpf calls
Related show

Commit Message

John Fastabend June 16, 2021, 10:55 p.m.
When populating poke_tab[] of a subprog we call map_poke_track() after
doing bpf_jit_add_poke_descriptor(). But, bpf_jit_add_poke_descriptor()
may, likely will, realloc the poke_tab[] structure and free the old
one. So that prog->aux->poke_tab is not stable. However, the aux pointer
is referenced from bpf_array_aux and poke_tab[] is used to 'track'
prog<->map link. This way when progs are released the entry in the
map is dropped and vice versa when the map is released we don't drop
it too soon if a prog is in the process of calling it.

I wasn't able to trigger any errors here, for example having map_poke_run
run with a poke_tab[] pointer that was free'd from
bpf_jit_add_poke_descriptor(), but it looks possible and at very least
is very fragile.

This patch moves poke_track call out of loop that is calling add_poke
so that we only ever add stable aux->poke_tab pointers to the map's
bpf_array_aux struct. Further, we need this in the next patch to fix
a real bug where progs are not 'untracked'.

Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
 kernel/bpf/verifier.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Alexei Starovoitov June 22, 2021, 9:54 p.m. | #1
On Wed, Jun 16, 2021 at 03:55:19PM -0700, John Fastabend wrote:
> When populating poke_tab[] of a subprog we call map_poke_track() after

> doing bpf_jit_add_poke_descriptor(). But, bpf_jit_add_poke_descriptor()

> may, likely will, realloc the poke_tab[] structure and free the old

> one. So that prog->aux->poke_tab is not stable. However, the aux pointer

> is referenced from bpf_array_aux and poke_tab[] is used to 'track'

> prog<->map link. This way when progs are released the entry in the

> map is dropped and vice versa when the map is released we don't drop

> it too soon if a prog is in the process of calling it.

> 

> I wasn't able to trigger any errors here, for example having map_poke_run

> run with a poke_tab[] pointer that was free'd from

> bpf_jit_add_poke_descriptor(), but it looks possible and at very least

> is very fragile.

> 

> This patch moves poke_track call out of loop that is calling add_poke

> so that we only ever add stable aux->poke_tab pointers to the map's

> bpf_array_aux struct. Further, we need this in the next patch to fix

> a real bug where progs are not 'untracked'.

> 

> Signed-off-by: John Fastabend <john.fastabend@gmail.com>

> ---

>  kernel/bpf/verifier.c |    6 +++++-

>  1 file changed, 5 insertions(+), 1 deletion(-)

> 

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c

> index 6e2ebcb0d66f..066fac9b5460 100644

> --- a/kernel/bpf/verifier.c

> +++ b/kernel/bpf/verifier.c

> @@ -12126,8 +12126,12 @@ static int jit_subprogs(struct bpf_verifier_env *env)

>  			}

>  

>  			func[i]->insnsi[insn_idx - subprog_start].imm = ret + 1;

> +		}

>  

> -			map_ptr = func[i]->aux->poke_tab[ret].tail_call.map;

> +		for (j = 0; j < func[i]->aux->size_poke_tab; j++) {

> +			int ret;

> +

> +			map_ptr = func[i]->aux->poke_tab[j].tail_call.map;


I don't see why it's necessary.
poke_tab pointer will be re-read after bpf_jit_add_poke_descriptor().
The compiler is not allowed to cache it.

I've applied the patch 1.
John Fastabend June 22, 2021, 10:59 p.m. | #2
Alexei Starovoitov wrote:
> On Wed, Jun 16, 2021 at 03:55:19PM -0700, John Fastabend wrote:

> > When populating poke_tab[] of a subprog we call map_poke_track() after

> > doing bpf_jit_add_poke_descriptor(). But, bpf_jit_add_poke_descriptor()

> > may, likely will, realloc the poke_tab[] structure and free the old

> > one. So that prog->aux->poke_tab is not stable. However, the aux pointer

> > is referenced from bpf_array_aux and poke_tab[] is used to 'track'

> > prog<->map link. This way when progs are released the entry in the

> > map is dropped and vice versa when the map is released we don't drop

> > it too soon if a prog is in the process of calling it.

> > 

> > I wasn't able to trigger any errors here, for example having map_poke_run

> > run with a poke_tab[] pointer that was free'd from

> > bpf_jit_add_poke_descriptor(), but it looks possible and at very least

> > is very fragile.

> > 

> > This patch moves poke_track call out of loop that is calling add_poke

> > so that we only ever add stable aux->poke_tab pointers to the map's

> > bpf_array_aux struct. Further, we need this in the next patch to fix

> > a real bug where progs are not 'untracked'.

> > 

> > Signed-off-by: John Fastabend <john.fastabend@gmail.com>

> > ---

> >  kernel/bpf/verifier.c |    6 +++++-

> >  1 file changed, 5 insertions(+), 1 deletion(-)

> > 

> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c

> > index 6e2ebcb0d66f..066fac9b5460 100644

> > --- a/kernel/bpf/verifier.c

> > +++ b/kernel/bpf/verifier.c

> > @@ -12126,8 +12126,12 @@ static int jit_subprogs(struct bpf_verifier_env *env)

> >  			}

> >  

> >  			func[i]->insnsi[insn_idx - subprog_start].imm = ret + 1;

> > +		}

> >  

> > -			map_ptr = func[i]->aux->poke_tab[ret].tail_call.map;

> > +		for (j = 0; j < func[i]->aux->size_poke_tab; j++) {

> > +			int ret;

> > +

> > +			map_ptr = func[i]->aux->poke_tab[j].tail_call.map;

> 

> I don't see why it's necessary.


Agree its not necessary. Nothing else can get at poke_tab while we do
the realloc so I agree its fine as is. It still seems odd to me to do the
poke_track when we know we are about to do multiple reallocs in the
next round of the loop. Either way I'll reply on the feedback in 3/4 and
we can avoid this patch altogether I think.

> poke_tab pointer will be re-read after bpf_jit_add_poke_descriptor().

> The compiler is not allowed to cache it.

> 

> I've applied the patch 1.

Patch

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 6e2ebcb0d66f..066fac9b5460 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -12126,8 +12126,12 @@  static int jit_subprogs(struct bpf_verifier_env *env)
 			}
 
 			func[i]->insnsi[insn_idx - subprog_start].imm = ret + 1;
+		}
 
-			map_ptr = func[i]->aux->poke_tab[ret].tail_call.map;
+		for (j = 0; j < func[i]->aux->size_poke_tab; j++) {
+			int ret;
+
+			map_ptr = func[i]->aux->poke_tab[j].tail_call.map;
 			ret = map_ptr->ops->map_poke_track(map_ptr, func[i]->aux);
 			if (ret < 0) {
 				verbose(env, "tracking tail call prog failed\n");