Re: [PATCH bpf-next v3 06/15] bpf: kernel side support for BTF Var and DataSec

[Martin Lau <kafai@fb.com>]

On Thu, Apr 04, 2019 at 07:20:51PM +0000, Martin Lau wrote:
> On Wed, Apr 03, 2019 at 08:22:57PM +0200, Daniel Borkmann wrote:
> > This work adds kernel-side verification, logging and seq_show dumping
> > of BTF Var and DataSec kinds which are emitted with latest LLVM. The
> > following constraints apply:
> > 
> > BTF Var must have:
> > 
> > - Its kind_flag is 0
> > - Its vlen is 0
> > - Must point to a valid type
> > - Type must not resolve to a forward type
> > - Must have a valid name
> > - Name may include dots (e.g. in case of static variables
> >   inside functions)
> > - Cannot be a member of a struct/union
> > - Linkage so far can either only be static or global/allocated
> > 
> > BTF DataSec must have:
> > 
> > - Its kind_flag is 0
> > - Its vlen cannot be 0
> > - Its size cannot be 0
> > - Must have a valid name
> > - Name may include dots (e.g. to represent .bss, .data, .rodata etc)
> > - Cannot be a member of a struct/union
> > - Inner btf_var_secinfo array with {type,offset,size} triple
> >   must be sorted by offset in ascending order
> > - Type must always point to BTF Var
> > - BTF resolved size of Var must be <= size provided by triple
> > - DataSec size must be >= sum of triple sizes (thus holes
> >   are allowed)
> Looks very good.  A few questions.
> 
> [ ... ]
> 
> > @@ -308,6 +320,7 @@ static bool btf_type_is_modifier(const struct btf_type *t)
> >  	case BTF_KIND_VOLATILE:
> >  	case BTF_KIND_CONST:
> >  	case BTF_KIND_RESTRICT:
> > +	case BTF_KIND_VAR:
> Why BTF_KIND_VAR is added here?
> 
> If possible, it may be better to keep is_modifier() as is since var
> intuitively does not belong to the is_modifier() test.
> 
> >  		return true;
> >  	}
> >  
> > @@ -375,13 +388,27 @@ static bool btf_type_is_int(const struct btf_type *t)
> >  	return BTF_INFO_KIND(t->info) == BTF_KIND_INT;
> >  }
> >  
> > +static bool btf_type_is_var(const struct btf_type *t)
> > +{
> > +	return BTF_INFO_KIND(t->info) == BTF_KIND_VAR;
> > +}
> > +
> > +static bool btf_type_is_datasec(const struct btf_type *t)
> > +{
> > +	return BTF_INFO_KIND(t->info) == BTF_KIND_DATASEC;
> > +}
> > +
> >  /* What types need to be resolved?
> >   *
> >   * btf_type_is_modifier() is an obvious one.
> >   *
> >   * btf_type_is_struct() because its member refers to
> >   * another type (through member->type).
> > -
> > + *
> > + * btf_type_is_var() because the variable refers to
> > + * another type. btf_type_is_datasec() holds multiple
> > + * btf_type_is_var() types that need resolving.
> > + *
> >   * btf_type_is_array() because its element (array->type)
> >   * refers to another type.  Array can be thought of a
> >   * special case of struct while array just has the same
> > @@ -390,9 +417,11 @@ static bool btf_type_is_int(const struct btf_type *t)
> >  static bool btf_type_needs_resolve(const struct btf_type *t)
> >  {
> >  	return btf_type_is_modifier(t) ||
> > -		btf_type_is_ptr(t) ||
> > -		btf_type_is_struct(t) ||
> > -		btf_type_is_array(t);
> > +	       btf_type_is_ptr(t) ||
> > +	       btf_type_is_struct(t) ||
> > +	       btf_type_is_array(t) ||
> > +	       btf_type_is_var(t) ||
> is_var() is also tested here on top of is_modifier().
> 
> > +	       btf_type_is_datasec(t);
> >  }
> >  
> >  /* t->size can be used */
> > @@ -403,6 +432,7 @@ static bool btf_type_has_size(const struct btf_type *t)
> >  	case BTF_KIND_STRUCT:
> >  	case BTF_KIND_UNION:
> >  	case BTF_KIND_ENUM:
> > +	case BTF_KIND_DATASEC:
> >  		return true;
> >  	}
> >  
> 
> [ ... ]
> 
> > +static int btf_var_resolve(struct btf_verifier_env *env,
> > +			   const struct resolve_vertex *v)
> > +{
> > +	const struct btf_type *next_type;
> > +	const struct btf_type *t = v->t;
> > +	u32 next_type_id = t->type;
> > +	struct btf *btf = env->btf;
> > +	u32 next_type_size;
> > +
> > +	next_type = btf_type_by_id(btf, next_type_id);
> Does the BTF verifier complain if BTF_KIND_VAR -> BTF_KIND_VAR -> BTF_KIND_INT?
> 
> The same goes for BTF_KIND_PTR -> BTF_KIND_VAR -> BTF_KIND_INT.
I was about to try but it seems I cannot apply the set cleanly.
Likely due to some recent changes.

After a quick skim through, the above cases seem possible.

If rejecting the above cases is desired,
I think it may be easier to check them at the individual
type's _resolve() instead of depending on the final resort
btf_resolve_valid().  The individual type's _resolve() should
know better what are the acceptable next_type[s].
I was thinking btf_resolve_valid() is more like a
"btf verifier internal error" to ensure the
earlier individual type's _resolve() is sane.

It seems at least the modifier_resolve() and ptr_resolve()
are missing to reject BTF_KIND_FUNC.  I will code
up a patch to fix BTF_KIND_FUNC.

> 
> > +	if (!next_type) {
> > +		btf_verifier_log_type(env, v->t, "Invalid type_id");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (!env_type_is_resolve_sink(env, next_type) &&
> > +	    !env_type_is_resolved(env, next_type_id))
> > +		return env_stack_push(env, next_type, next_type_id);
> > +
> > +	if (btf_type_is_modifier(next_type)) {
> May be a few comments on why this is needed.  Together with
> the is_modifier() change above, it is not immediately clear to me.
> 
> I suspect this could be left to be done in the btf_ptr_resolve()
> as well if the ptr type happens to be behind the var type in
> the ".BTF" ELF.
> 
> > +		const struct btf_type *resolved_type;
> > +		u32 resolved_type_id;
> > +
> > +		resolved_type_id = next_type_id;
> > +		resolved_type = btf_type_id_resolve(btf, &resolved_type_id);
> > +
> > +		if (btf_type_is_ptr(resolved_type) &&
> > +		    !env_type_is_resolve_sink(env, resolved_type) &&
> > +		    !env_type_is_resolved(env, resolved_type_id))
> > +			return env_stack_push(env, resolved_type,
> > +					      resolved_type_id);
> > +	}
> > +
> > +	/* We must resolve to something concrete at this point, no
> > +	 * forward types or similar that would resolve to size of
> > +	 * zero is allowed.
> > +	 */
> > +	if (!btf_type_id_size(btf, &next_type_id, &next_type_size)) {
> > +		btf_verifier_log_type(env, v->t, "Invalid type_id");
> > +		return -EINVAL;
> > +	}
> > +
> > +	env_stack_pop_resolved(env, next_type_id, next_type_size);
> > +
> > +	return 0;
> > +}
> > +
> 
> [ ... ]
> 
> > @@ -2622,13 +2983,17 @@ static bool btf_resolve_valid(struct btf_verifier_env *env,
> >  	if (!env_type_is_resolved(env, type_id))
> >  		return false;
> >  
> > -	if (btf_type_is_struct(t))
> > +	if (btf_type_is_struct(t) || btf_type_is_datasec(t))
> >  		return !btf->resolved_ids[type_id] &&
> > -			!btf->resolved_sizes[type_id];
> > +		       !btf->resolved_sizes[type_id];
> >  
> > -	if (btf_type_is_modifier(t) || btf_type_is_ptr(t)) {
> > +	if (btf_type_is_modifier(t) || btf_type_is_ptr(t) ||
> > +	    btf_type_is_var(t)) {
> >  		t = btf_type_id_resolve(btf, &type_id);
> > -		return t && !btf_type_is_modifier(t);
> > +		return t &&
> > +		       !btf_type_is_modifier(t) &&
> > +		       !btf_type_is_var(t) &&
> > +		       !btf_type_is_datasec(t);
> >  	}
> >  
> >  	if (btf_type_is_array(t)) {
> > -- 
> > 2.9.5
> >