by Yonghong Song

[permalink] [raw]

Subject: Re: [PATCH v2 bpf-next 2/7] bpf: move to generic BTF show support, apply it to seq files/strings

On 5/18/20 2:46 AM, Alan Maguire wrote:
> On Wed, 13 May 2020, Yonghong Song wrote:
>
>>
>>> +struct btf_show {
>>> + u64 flags;
>>> + void *target; /* target of show operation (seq file, buffer) */
>>> + void (*showfn)(struct btf_show *show, const char *fmt, ...);
>>> + const struct btf *btf;
>>> + /* below are used during iteration */
>>> + struct {
>>> + u8 depth;
>>> + u8 depth_shown;
>>> + u8 depth_check;
>>
>> I have some difficulties to understand the relationship between
>> the above three variables. Could you add some comments here?
>>
>
> Will do; sorry the code got a bit confusing. The goal is to track
> which sub-components in a data structure we need to display. The
> "depth" variable tracks where we are currently; "depth_shown"
> is the depth at which we have something nonzer to display (perhaps
> "depth_to_show" would be a better name?). "depth_check" tells

"depth_to_show" is indeed better.

> us whether we are currently checking depth or doing printing.
> If we're checking, we don't actually print anything, we merely note
> if we hit a non-zero value, and if so, we set "depth_shown"
> to the depth at which we hit that value.
>
> When we show a struct, union or array, we will only display an
> object has one or more non-zero members. But because
> the struct can in turn nest a struct or array etc, we need
> to recurse into the object. When we are doing that, depth_check
> is set, and this tells us not to do any actual display. When
> that recursion is complete, we check if "depth_shown" (depth
> to show) is > depth (i.e. we found something) and if it is
> we go on to display the object (setting depth_check to 0).

Thanks for the explanation. Putting them in the comments
will be great.

>
> There may be a better way to solve this problem of course,
> but I wanted to avoid storing values where possible as
> deeply-nested data structures might overrun such storage.
>
[...]
>>> +
>>> + /*
>>> + * The goal here is to build up the right number of pointer and
>>> + * array suffixes while ensuring the type name for a typedef
>>> + * is represented. Along the way we accumulate a list of
>>> + * BTF kinds we have encountered, since these will inform later
>>> + * display; for example, pointer types will not require an
>>> + * opening "{" for struct, we will just display the pointer value.
>>> + *
>>> + * We also want to accumulate the right number of pointer or array
>>> + * indices in the format string while iterating until we get to
>>> + * the typedef/pointee/array member target type.
>>> + *
>>> + * We start by pointing at the end of pointer and array suffix
>>> + * strings; as we accumulate pointers and arrays we move the pointer
>>> + * or array string backwards so it will show the expected number of
>>> + * '*' or '[]' for the type. BTF_SHOW_MAX_ITER of nesting of pointers
>>> + * and/or arrays and typedefs are supported as a precaution.
>>> + *
>>> + * We also want to get typedef name while proceeding to resolve
>>> + * type it points to so that we can add parentheses if it is a
>>> + * "typedef struct" etc.
>>> + */
>>> + for (i = 0; i < BTF_SHOW_MAX_ITER; i++) {
>>> +
>>> + switch (BTF_INFO_KIND(t->info)) {
>>> + case BTF_KIND_TYPEDEF:
>>> + if (!type_name)
>>> + type_name = btf_name_by_offset(show->btf,
>>> + t->name_off);
type_name should never be NULL for valid vmlinux BTF.

>>> + kinds |= BTF_KIND_BIT(BTF_KIND_TYPEDEF);
>>> + id = t->type;
>>> + break;
>>> + case BTF_KIND_ARRAY:
>>> + kinds |= BTF_KIND_BIT(BTF_KIND_ARRAY);
>>> + parens = "[";
>>> + array = btf_type_array(t);
>>> + if (!array)
array will never be NULL here.
>>> + return show->state.type_name;
>>> + if (!t)
t will never be NULL here.
>>> + return show->state.type_name;
>>> + if (array_suffix > array_suffixes)
>>> + array_suffix -= 2;
>>> + id = array->type;
>>> + break;
>>> + case BTF_KIND_PTR:
>>> + kinds |= BTF_KIND_BIT(BTF_KIND_PTR);
>>> + if (ptr_suffix > ptr_suffixes)
>>> + ptr_suffix -= 1;
>>> + id = t->type;
>>> + break;
>>> + default:
>>> + id = 0;
>>> + break;
>>> + }
>>> + if (!id)
>>> + break;
>>> + t = btf_type_skip_qualifiers(show->btf, id);
t should never be NULL here.
>>> + if (!t)
>>> + return show->state.type_name;
>>> + }
>>
>> Do we do pointer tracing here? For example
>> struct t {
>> int *m[5];
>> }
>>
>> When trying to access memory, the above code may go through
>> ptr->array and out of loop when hitting array element type "int"?
>>
>
> I'm not totally sure I understand the question so I'll
> try and describe how the above is supposed to work. I
> think there's a bug here alright.
>
> In the above case, when we reach the "m" field of "struct t",
> the code should start with the BTF_KIND_ARRAY, set up
> the array suffix, then get the array type which is a PTR
> and we will set up the ptr suffix to be "*" and we set
> the id to the id associated with "int", and
> btf_type_skip_qualifiers() will use that id to look up
> the new value for the type used in btf_name_by_offset().
> So on the next iteration we hit the int itself and bail from
> the loop, having noted that we've got a _PTR and _ARRAY set in
> the "kinds" bitfield.
>
> Then we look up the int type using "t" with btf_name_by_offset,
> so we end up displaying "(int *m[])" as the type.

Thanks for explanation. Previously I thought this somehow
may be related to tracing data. Looks it is only for
*constructing* type names. So it largely looks fine though.

>
> However the code assumes we don't need the parentheses for
> the array if we have encountered a pointer; that's never
> the case. We only should eliminate the opening parens
> for a struct or union "{" in such cases, as in those cases
> we have a pointer to the struct rather than a nested struct.
> So that needs to be fixed. Are the other problems here you're
> seeing that the above doesn't cover?

A few minor comments in the above.

>
>>> + /* We may not be able to represent this type; bail to be safe */
>>> + if (i == BTF_SHOW_MAX_ITER)
>>> + return show->state.type_name;
>>> +
>>> + if (!type_name)
>>> + type_name = btf_name_by_offset(show->btf, t->name_off);
>>> +
>>> + switch (BTF_INFO_KIND(t->info)) {
>>> + case BTF_KIND_STRUCT:
>>> + case BTF_KIND_UNION:
>>> + prefix = BTF_INFO_KIND(t->info) == BTF_KIND_STRUCT ?
>>> + "struct" : "union";
>>> + /* if it's an array of struct/union, parens is already set */
>>> + if (!(kinds & (BTF_KIND_BIT(BTF_KIND_ARRAY))))
>>> + parens = "{";
>>> + break;
>>> + case BTF_KIND_ENUM:
>>> + prefix = "enum";
>>> + break;
>>> + default:
>>> + allow_anon = false;
[...]
>>> + if (elem_type && btf_type_is_int(elem_type)) {
>>> + u32 int_type = btf_type_int(elem_type);
>>> +
>>> + encoding = BTF_INT_ENCODING(int_type);
>>> +
>>> + /*
>>> + * BTF_INT_CHAR encoding never seems to be set for
>>> + * char arrays, so if size is 1 and element is
>>> + * printable as a char, we'll do that.
>>> + */
>>> + if (elem_size == 1) > + encoding =
>>> BTF_INT_CHAR;
>>
>> Some char array may be printable and some may not be printable,
>> how did you differentiate this?
>>
>
> I should probably change the logic to ensure all chars
> (before a \0) are printable. I'll do that for v2. We will always
> have cases (e.g. the skb cb[] field) where the char[] is not
> intended as a string, but I think the utility of showing them as
> strings where possible is worthwhile.

Make sense. Thanks!

>
> Thanks again for reviewing!
>
> Alan
>