| Message ID | 162388411986.151936.3914295553899556046.stgit@john-XPS-13-9370 (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | BPF fixes mixed tail and bpf2bpf calls | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for bpf |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 6 maintainers not CCed: yhs@fb.com kpsingh@kernel.org bpf@vger.kernel.org andrii@kernel.org kafai@fb.com songliubraving@fb.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 20 this patch: 20 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 13 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 20 this patch: 20 |
| netdev/header_inline | success | Link |
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> > --- Needs a fixes tag, Fixes: a748c6975dea3 ("bpf: propagate poke descriptors to subprograms")
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.
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.
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");
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(-)