[PATCH bpf-next] bpf: btf: restore resolve_mode when popping the resolve stack

[PATCH bpf-next] bpf: btf: restore resolve_mode when popping the resolve stack

[Lorenz Bauer]

In commit 9b459804ff99 ("btf: fix resolving BTF_KIND_VAR after ARRAY, STRUCT, UNION, PTR")
I fixed a bug that occurred during resolving of a DATASEC by strategically resetting
resolve_mode. This fixes the immediate bug but leaves us open to future bugs where
nested types have to be resolved.

The problem is that env_stack_pop_resolved never restores the previously active
resolve mode when discarding a stack item. Fix this by adding the previous resolve
mode to each resolve_vertex and updating env->resolve_mode during pop.

Signed-off-by: Lorenz Bauer <lmb@isovalent.com>
---
 kernel/bpf/btf.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 6b682b8e4b50..4d6c1d0e8b7c 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -264,10 +264,19 @@ enum verifier_phase {
 	CHECK_TYPE,
 };
 
+enum resolve_mode {
+	RESOLVE_TBD,	/* To Be Determined */
+	RESOLVE_PTR,	/* Resolving for Pointer */
+	RESOLVE_STRUCT_OR_ARRAY,	/* Resolving for struct/union
+					 * or array
+					 */
+};
+
 struct resolve_vertex {
 	const struct btf_type *t;
 	u32 type_id;
 	u16 next_member;
+	enum resolve_mode parent_mode;
 };
 
 enum visit_state {
@@ -276,13 +285,6 @@ enum visit_state {
 	RESOLVED,
 };
 
-enum resolve_mode {
-	RESOLVE_TBD,	/* To Be Determined */
-	RESOLVE_PTR,	/* Resolving for Pointer */
-	RESOLVE_STRUCT_OR_ARRAY,	/* Resolving for struct/union
-					 * or array
-					 */
-};
 
 #define MAX_RESOLVE_DEPTH 32
 
@@ -1811,6 +1813,7 @@ static int env_stack_push(struct btf_verifier_env *env,
 	v->t = t;
 	v->type_id = type_id;
 	v->next_member = 0;
+	v->parent_mode = env->resolve_mode;
 
 	if (env->resolve_mode == RESOLVE_TBD) {
 		if (btf_type_is_ptr(t))
@@ -1832,13 +1835,15 @@ static void env_stack_pop_resolved(struct btf_verifier_env *env,
 				   u32 resolved_type_id,
 				   u32 resolved_size)
 {
-	u32 type_id = env->stack[--(env->top_stack)].type_id;
+	struct resolve_vertex *v = &env->stack[--(env->top_stack)];
+	u32 type_id = v->type_id;
 	struct btf *btf = env->btf;
 
 	type_id -= btf->start_id; /* adjust to local type id */
 	btf->resolved_sizes[type_id] = resolved_size;
 	btf->resolved_ids[type_id] = resolved_type_id;
 	env->visit_states[type_id] = RESOLVED;
+	env->resolve_mode = v->parent_mode;
 }
 
 static const struct resolve_vertex *env_stack_peak(struct btf_verifier_env *env)
@@ -4541,7 +4546,6 @@ static int btf_datasec_resolve(struct btf_verifier_env *env,
 	struct btf *btf = env->btf;
 	u16 i;
 
-	env->resolve_mode = RESOLVE_TBD;
 	for_each_vsi_from(i, v->next_member, v->t, vsi) {
 		u32 var_type_id = vsi->type, type_id, type_size = 0;
 		const struct btf_type *var_type = btf_type_by_id(env->btf,
-- 
2.40.1

Re: [PATCH bpf-next] bpf: btf: restore resolve_mode when popping the resolve stack

[Daniel Borkmann]

On 5/15/23 2:15 PM, Lorenz Bauer wrote:
> In commit 9b459804ff99 ("btf: fix resolving BTF_KIND_VAR after ARRAY, STRUCT, UNION, PTR")
> I fixed a bug that occurred during resolving of a DATASEC by strategically resetting
> resolve_mode. This fixes the immediate bug but leaves us open to future bugs where
> nested types have to be resolved.

Lgtm, is there a way we could also craft a test case for this corner case?

Thanks,
Daniel

Re: [PATCH bpf-next] bpf: btf: restore resolve_mode when popping the resolve stack

[Lorenz Bauer]

On Mon, May 15, 2023 at 8:26 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 5/15/23 2:15 PM, Lorenz Bauer wrote:
> > In commit 9b459804ff99 ("btf: fix resolving BTF_KIND_VAR after ARRAY, STRUCT, UNION, PTR")
> > I fixed a bug that occurred during resolving of a DATASEC by strategically resetting
> > resolve_mode. This fixes the immediate bug but leaves us open to future bugs where
> > nested types have to be resolved.
>
> Lgtm, is there a way we could also craft a test case for this corner case?

There is a test for the datasec bug already, it went in with the
original patch. See commit dfdd608c3b36 ("selftests/bpf: check that
modifier resolves after pointer").

Not sure how to test this beyond that specific case.

Re: [PATCH bpf-next] bpf: btf: restore resolve_mode when popping the resolve stack

[Martin KaFai Lau]

On 5/15/23 5:15 AM, Lorenz Bauer wrote:
> In commit 9b459804ff99 ("btf: fix resolving BTF_KIND_VAR after ARRAY, STRUCT, UNION, PTR")
> I fixed a bug that occurred during resolving of a DATASEC by strategically resetting
> resolve_mode. This fixes the immediate bug but leaves us open to future bugs where
> nested types have to be resolved.

hmm... future bugs like when adding new BTF_KIND in the future?

> 
> The problem is that env_stack_pop_resolved never restores the previously active
> resolve mode when discarding a stack item. Fix this by adding the previous resolve
> mode to each resolve_vertex and updating env->resolve_mode during pop.
> 
> Signed-off-by: Lorenz Bauer <lmb@isovalent.com>
> ---
>   kernel/bpf/btf.c | 22 +++++++++++++---------
>   1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 6b682b8e4b50..4d6c1d0e8b7c 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -264,10 +264,19 @@ enum verifier_phase {
>   	CHECK_TYPE,
>   };
>   
> +enum resolve_mode {
> +	RESOLVE_TBD,	/* To Be Determined */
> +	RESOLVE_PTR,	/* Resolving for Pointer */
> +	RESOLVE_STRUCT_OR_ARRAY,	/* Resolving for struct/union
> +					 * or array
> +					 */
> +};
> +
>   struct resolve_vertex {
>   	const struct btf_type *t;
>   	u32 type_id;
>   	u16 next_member;
> +	enum resolve_mode parent_mode;
>   };
>   
>   enum visit_state {
> @@ -276,13 +285,6 @@ enum visit_state {
>   	RESOLVED,
>   };
>   
> -enum resolve_mode {
> -	RESOLVE_TBD,	/* To Be Determined */
> -	RESOLVE_PTR,	/* Resolving for Pointer */
> -	RESOLVE_STRUCT_OR_ARRAY,	/* Resolving for struct/union
> -					 * or array
> -					 */
> -};
>   
>   #define MAX_RESOLVE_DEPTH 32
>   
> @@ -1811,6 +1813,7 @@ static int env_stack_push(struct btf_verifier_env *env,
>   	v->t = t;
>   	v->type_id = type_id;
>   	v->next_member = 0;
> +	v->parent_mode = env->resolve_mode;
>   
>   	if (env->resolve_mode == RESOLVE_TBD) {
>   		if (btf_type_is_ptr(t))
> @@ -1832,13 +1835,15 @@ static void env_stack_pop_resolved(struct btf_verifier_env *env,
>   				   u32 resolved_type_id,
>   				   u32 resolved_size)
>   {
> -	u32 type_id = env->stack[--(env->top_stack)].type_id;
> +	struct resolve_vertex *v = &env->stack[--(env->top_stack)];
> +	u32 type_id = v->type_id;
>   	struct btf *btf = env->btf;
>   
>   	type_id -= btf->start_id; /* adjust to local type id */
>   	btf->resolved_sizes[type_id] = resolved_size;
>   	btf->resolved_ids[type_id] = resolved_type_id;
>   	env->visit_states[type_id] = RESOLVED;
> +	env->resolve_mode = v->parent_mode;

Other than datasec, could v->parent_mode and env->resolve_mode be different 
while resolving? I would prefer to keep the 'env->resolve_mode = RESOLVE_TBD' in 
btf_datasec_resolve() to make this special case clear.

>   }
>   
>   static const struct resolve_vertex *env_stack_peak(struct btf_verifier_env *env)
> @@ -4541,7 +4546,6 @@ static int btf_datasec_resolve(struct btf_verifier_env *env,
>   	struct btf *btf = env->btf;
>   	u16 i;
>   
> -	env->resolve_mode = RESOLVE_TBD;
>   	for_each_vsi_from(i, v->next_member, v->t, vsi) {
>   		u32 var_type_id = vsi->type, type_id, type_size = 0;
>   		const struct btf_type *var_type = btf_type_by_id(env->btf,

Re: [PATCH bpf-next] bpf: btf: restore resolve_mode when popping the resolve stack

[Lorenz Bauer]

On Wed, May 17, 2023 at 7:26 AM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>
> On 5/15/23 5:15 AM, Lorenz Bauer wrote:
> > In commit 9b459804ff99 ("btf: fix resolving BTF_KIND_VAR after ARRAY, STRUCT, UNION, PTR")
> > I fixed a bug that occurred during resolving of a DATASEC by strategically resetting
> > resolve_mode. This fixes the immediate bug but leaves us open to future bugs where
> > nested types have to be resolved.
>
> hmm... future bugs like when adding new BTF_KIND in the future?

It could just be refactoring of the codebase? What is the downside of
restoring the mode when popping the item? It also makes push and pop
symmetrical. Feel free to NACK if you don't want this change, not
going to push for it.

Re: [PATCH bpf-next] bpf: btf: restore resolve_mode when popping the resolve stack

[Martin KaFai Lau]

On 5/17/23 2:01 AM, Lorenz Bauer wrote:
> On Wed, May 17, 2023 at 7:26 AM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>>
>> On 5/15/23 5:15 AM, Lorenz Bauer wrote:
>>> In commit 9b459804ff99 ("btf: fix resolving BTF_KIND_VAR after ARRAY, STRUCT, UNION, PTR")
>>> I fixed a bug that occurred during resolving of a DATASEC by strategically resetting
>>> resolve_mode. This fixes the immediate bug but leaves us open to future bugs where
>>> nested types have to be resolved.
>>
>> hmm... future bugs like when adding new BTF_KIND in the future?
> 
> It could just be refactoring of the codebase? What is the downside of
> restoring the mode when popping the item? It also makes push and pop
> symmetrical.

I can see your point to refactor it to make it work for all different BTF_KIND.

Other than BTF_KIND_DATASEC, env->resolve_mode stays the same for all other 
kinds once it is decided. It is why resolve_mode is in the "env" instead of "v". 
My concern is this will hide some bugs (existing or future) that accidentally 
changed the resolve_mode in the middle. If there is another legit case that 
could be found other than BTF_KIND_DATASEC, that will be a better time to do 
this refactoring with a proper test case considering most bpf progs need btf to 
load nowadays and probably need to veristat test also. If it came to that, might 
as well consider moving resolve_mode from "env" to "v".

btf_datasec_resolve() is acting as a very top level resolver like btf_resolve(), 
so it reset env->resolve_mode before resolving its var member like how 
btf_resolve() does. imo, together with env->resolve_mode stays the same for 
others, it is more straight forward to reason. I understand that it is personal 
preference and could argue either way.

Re: [PATCH bpf-next] bpf: btf: restore resolve_mode when popping the resolve stack

[Lorenz Bauer]

On Thu, May 18, 2023 at 2:42 AM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>
> On 5/17/23 2:01 AM, Lorenz Bauer wrote:
> > On Wed, May 17, 2023 at 7:26 AM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>
> I can see your point to refactor it to make it work for all different BTF_KIND.
>
> Other than BTF_KIND_DATASEC, env->resolve_mode stays the same for all other
> kinds once it is decided. It is why resolve_mode is in the "env" instead of "v".
> My concern is this will hide some bugs (existing or future) that accidentally
> changed the resolve_mode in the middle. If there is another legit case that
> could be found other than BTF_KIND_DATASEC, that will be a better time to do
> this refactoring with a proper test case considering most bpf progs need btf to
> load nowadays and probably need to veristat test also. If it came to that, might
> as well consider moving resolve_mode from "env" to "v".
>
> btf_datasec_resolve() is acting as a very top level resolver like btf_resolve(),
> so it reset env->resolve_mode before resolving its var member like how
> btf_resolve() does. imo, together with env->resolve_mode stays the same for
> others, it is more straight forward to reason. I understand that it is personal
> preference and could argue either way.

 Okay, let's drop it then :)