| Message ID | 1630564633-552375-1-git-send-email-jiasheng@iscas.ac.cn (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: Add env_type_is_resolved() in front of env_stack_push() in btf_resolve() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Thu, Sep 02, 2021 at 06:37:13AM +0000, jiasheng wrote: > We have found that in the complied files env_stack_push() > appear more than 10 times, and under at least 90% circumstances > that env_type_is_resolved() and env_stack_push() appear in pairs. > For example, they appear together in the btf_modifier_resolve() > of the file complie from 'kernel/bpf/btf.c'. > But we have found that in the btf_resolve(), there is only > env_stack_push() instead of the pair. > Therefore, we consider that the env_type_is_resolved() > might be forgotten. It does not justify a change like this just because one of its usage looks different and then concluded that it _might_ be forgotten. Does it have a bug or not? If there is, please provide an explanation on how to reproduce it first. > > Signed-off-by: jiasheng <jiasheng@iscas.ac.cn> > --- > kernel/bpf/btf.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > index f982a9f0..454c249 100644 > --- a/kernel/bpf/btf.c > +++ b/kernel/bpf/btf.c > @@ -4002,7 +4002,8 @@ static int btf_resolve(struct btf_verifier_env *env, > int err = 0; > > env->resolve_mode = RESOLVE_TBD; > - env_stack_push(env, t, type_id); > + if (env_type_is_resolved(env, type_id)) > + env_stack_push(env, t, type_id); > while (!err && (v = env_stack_peak(env))) { > env->log_type_id = v->type_id; > err = btf_type_ops(v->t)->resolve(env, v); > -- > 2.7.4 >
On Tue, Sep 7, 2021 at 3:37 PM Martin KaFai Lau <kafai@fb.com> wrote: > > On Thu, Sep 02, 2021 at 06:37:13AM +0000, jiasheng wrote: > > We have found that in the complied files env_stack_push() > > appear more than 10 times, and under at least 90% circumstances > > that env_type_is_resolved() and env_stack_push() appear in pairs. > > For example, they appear together in the btf_modifier_resolve() > > of the file complie from 'kernel/bpf/btf.c'. > > But we have found that in the btf_resolve(), there is only > > env_stack_push() instead of the pair. > > Therefore, we consider that the env_type_is_resolved() > > might be forgotten. > It does not justify a change like this just because > one of its usage looks different and then concluded that > it _might_ be forgotten. > > Does it have a bug or not? If there is, please > provide an explanation on how to reproduce it first. > Both places that call btf_resolve() check that env_type_is_resolved() first, so there is no issue here. > > > > Signed-off-by: jiasheng <jiasheng@iscas.ac.cn> > > --- > > kernel/bpf/btf.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > > index f982a9f0..454c249 100644 > > --- a/kernel/bpf/btf.c > > +++ b/kernel/bpf/btf.c > > @@ -4002,7 +4002,8 @@ static int btf_resolve(struct btf_verifier_env *env, > > int err = 0; > > > > env->resolve_mode = RESOLVE_TBD; > > - env_stack_push(env, t, type_id); > > + if (env_type_is_resolved(env, type_id)) > > + env_stack_push(env, t, type_id); > > while (!err && (v = env_stack_peak(env))) { > > env->log_type_id = v->type_id; > > err = btf_type_ops(v->t)->resolve(env, v); > > -- > > 2.7.4 > >
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c index f982a9f0..454c249 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c @@ -4002,7 +4002,8 @@ static int btf_resolve(struct btf_verifier_env *env, int err = 0; env->resolve_mode = RESOLVE_TBD; - env_stack_push(env, t, type_id); + if (env_type_is_resolved(env, type_id)) + env_stack_push(env, t, type_id); while (!err && (v = env_stack_peak(env))) { env->log_type_id = v->type_id; err = btf_type_ops(v->t)->resolve(env, v);
We have found that in the complied files env_stack_push() appear more than 10 times, and under at least 90% circumstances that env_type_is_resolved() and env_stack_push() appear in pairs. For example, they appear together in the btf_modifier_resolve() of the file complie from 'kernel/bpf/btf.c'. But we have found that in the btf_resolve(), there is only env_stack_push() instead of the pair. Therefore, we consider that the env_type_is_resolved() might be forgotten. Signed-off-by: jiasheng <jiasheng@iscas.ac.cn> --- kernel/bpf/btf.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)