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