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

[Yonghong Song]

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