Hey Martin,
Thanks a lot for the feedback, much appreciated!
On 04/05/2019 09:03 AM, Martin Lau wrote:
> 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.
Hmm, agree, it's probably better to keep btf_type_is_modifier() as-is,
I might have been mislead by the comment in that function. Need to
double check whether that could trigger the WARN in btf_type_id_size().
>>> 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().
Yeah, correct.
>>> + 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.
The tests currently only check whether we end up at BTF_KIND_VAR or
BTF_KIND_DATASEC, but as far as I'm aware would accept some intermediate
BTF_KIND_VAR type, which should be better rejected instead.
> 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].
Makes sense, I'll change that today and add more test coverage for
this particular case. Moving the check into individual type's
_resolve() handler is indeed probably the best we can do at this
point, though unfortunate duplication.
> 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.
Okay.
>>> + 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.
Yep, the change in is_modifier() needs to be dropped.
>> 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
>>>
