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.
> + 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
>
