Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <a29c604e-5a68-eed2-b581-0ad4687fda10@linux.dev>
Date:   Tue, 16 May 2023 23:26:50 -0700
MIME-Version: 1.0
Subject: Re: [PATCH bpf-next] bpf: btf: restore resolve_mode when popping the
 resolve stack
Content-Language: en-US
To:     Lorenz Bauer <lmb@isovalent.com>
Cc:     bpf@vger.kernel.org, linux-kernel@vger.kernel.org,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>,
        Song Liu <song@kernel.org>, Yonghong Song <yhs@fb.com>,
        John Fastabend <john.fastabend@gmail.com>,
        KP Singh <kpsingh@kernel.org>,
        Stanislav Fomichev <sdf@google.com>,
        Hao Luo <haoluo@google.com>, Jiri Olsa <jolsa@kernel.org>
References: <20230515121521.30569-1-lmb@isovalent.com>
From:   Martin KaFai Lau <martin.lau@linux.dev>
In-Reply-To: <20230515121521.30569-1-lmb@isovalent.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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,