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

[Yonghong Song <yhs@fb.com>]

On 5/11/20 10:56 PM, Alan Maguire wrote:
> generalize the "seq_show" seq file support in btf.c to support
> a generic show callback of which we support two instances; the
> current seq file show, and a show with snprintf() behaviour which
> instead writes the type data to a supplied string.
> 
> Both classes of show function call btf_type_show() with different
> targets; the seq file or the string to be written.  In the string
> case we need to track additional data - length left in string to write
> and length to return that we would have written (a la snprintf).
> 
> By default show will display type information, field members and
> their types and values etc, and the information is indented
> based upon structure depth.
> 
> Show however supports flags which modify its behaviour:
> 
> BTF_SHOW_COMPACT - suppress newline/indent.
> BTF_SHOW_NONAME - suppress show of type and member names.
> BTF_SHOW_PTR_RAW - do not obfuscate pointer values.
> BTF_SHOW_ZERO - show zeroed values (by default they are not shown).
> 
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---
>   include/linux/btf.h |  33 +++
>   kernel/bpf/btf.c    | 759 +++++++++++++++++++++++++++++++++++++++++++++-------
>   2 files changed, 690 insertions(+), 102 deletions(-)
> 
> diff --git a/include/linux/btf.h b/include/linux/btf.h
> index 5c1ea99..d571125 100644
> --- a/include/linux/btf.h
> +++ b/include/linux/btf.h
> @@ -13,6 +13,7 @@
>   struct btf_member;
>   struct btf_type;
>   union bpf_attr;
> +struct btf_show;
>   
>   extern const struct file_operations btf_fops;
>   
> @@ -46,8 +47,40 @@ int btf_get_info_by_fd(const struct btf *btf,
>   const struct btf_type *btf_type_id_size(const struct btf *btf,
>   					u32 *type_id,
>   					u32 *ret_size);
> +
> +/*
> + * Options to control show behaviour.
> + *	- BTF_SHOW_COMPACT: no formatting around type information
> + *	- BTF_SHOW_NONAME: no struct/union member names/types
> + *	- BTF_SHOW_PTR_RAW: show raw (unobfuscated) pointer values;
> + *	  equivalent to %px.
> + *	- BTF_SHOW_ZERO: show zero-valued struct/union members; they
> + *	  are not displayed by default
> + */
> +#define BTF_SHOW_COMPACT	(1ULL << 0)
> +#define BTF_SHOW_NONAME		(1ULL << 1)
> +#define BTF_SHOW_PTR_RAW	(1ULL << 2)
> +#define BTF_SHOW_ZERO		(1ULL << 3)
> +
>   void btf_type_seq_show(const struct btf *btf, u32 type_id, void *obj,
>   		       struct seq_file *m);
> +
> +/*
> + * Copy len bytes of string representation of obj of BTF type_id into buf.
> + *
> + * @btf: struct btf object
> + * @type_id: type id of type obj points to
> + * @obj: pointer to typed data
> + * @buf: buffer to write to
> + * @len: maximum length to write to buf
> + * @flags: show options (see above)
> + *
> + * Return: length that would have been/was copied as per snprintf, or
> + *	   negative error.
> + */
> +int btf_type_snprintf_show(const struct btf *btf, u32 type_id, void *obj,
> +			   char *buf, int len, u64 flags);
> +
>   int btf_get_fd_by_id(u32 id);
>   u32 btf_id(const struct btf *btf);
>   bool btf_member_is_reg_int(const struct btf *btf, const struct btf_type *s,
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index dcd2331..edf6455 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -281,6 +281,32 @@ static const char *btf_type_str(const struct btf_type *t)
>   	return btf_kind_str[BTF_INFO_KIND(t->info)];
>   }
>   
> +/*
> + * Common data to all BTF show operations. Private show functions can add
> + * their own data to a structure containing a struct btf_show and consult it
> + * in the show callback.  See btf_type_show() below.
> + */
> +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?

> +		u8 array_member:1,
> +		   array_terminated:1;
> +		u16 array_encoding;
> +		u32 type_id;
> +		const struct btf_type *type;
> +		const struct btf_member *member;
> +		char name[KSYM_NAME_LEN];	/* scratch space for name */
> +		char type_name[KSYM_NAME_LEN];	/* scratch space for type */

KSYM_NAME_LEN is for symbol name, not for type name. But I guess in 
kernel we probably do not have > 128 bytes type name so we should be
okay here.

> +	} state;
> +};
> +
>   struct btf_kind_operations {
>   	s32 (*check_meta)(struct btf_verifier_env *env,
>   			  const struct btf_type *t,
> @@ -297,9 +323,9 @@ struct btf_kind_operations {
>   				  const struct btf_type *member_type);
>   	void (*log_details)(struct btf_verifier_env *env,
>   			    const struct btf_type *t);
> -	void (*seq_show)(const struct btf *btf, const struct btf_type *t,
> +	void (*show)(const struct btf *btf, const struct btf_type *t,
>   			 u32 type_id, void *data, u8 bits_offsets,
> -			 struct seq_file *m);
> +			 struct btf_show *show);
>   };
>   
>   static const struct btf_kind_operations * const kind_ops[NR_BTF_KINDS];
> @@ -676,6 +702,340 @@ bool btf_member_is_reg_int(const struct btf *btf, const struct btf_type *s,
>   	return true;
>   }
>   
> +/* Similar to btf_type_skip_modifiers() but does not skip typedefs. */
> +static inline
> +const struct btf_type *btf_type_skip_qualifiers(const struct btf *btf, u32 id)
> +{
> +	const struct btf_type *t = btf_type_by_id(btf, id);
> +
> +	while (btf_type_is_modifier(t) &&
> +	       BTF_INFO_KIND(t->info) != BTF_KIND_TYPEDEF) {
> +		id = t->type;
> +		t = btf_type_by_id(btf, t->type);
> +	}
> +
> +	return t;
> +}
> +
> +#define BTF_SHOW_MAX_ITER	10
> +
> +#define BTF_KIND_BIT(kind)	(1ULL << kind)
> +
> +static inline const char *btf_show_type_name(struct btf_show *show,
> +					     const struct btf_type *t)
> +{
> +	const char *array_suffixes = "[][][][][][][][][][]";

Add a comment here saying length BTF_SHOW_MAX_ITER * 2
so later on if somebody changes the BTF_SHOW_MAX_ITER from 10 to 12,
it won't miss here?

> +	const char *array_suffix = &array_suffixes[strlen(array_suffixes)];
> +	const char *ptr_suffixes = "**********";

The same here.

> +	const char *ptr_suffix = &ptr_suffixes[strlen(ptr_suffixes)];
> +	const char *type_name = NULL, *prefix = "", *parens = "";
> +	const struct btf_array *array;
> +	u32 id = show->state.type_id;
> +	bool allow_anon = true;
> +	u64 kinds = 0;
> +	int i;
> +
> +	show->state.type_name[0] = '\0';
> +
> +	/*
> +	 * Start with type_id, as we have have resolved the struct btf_type *
> +	 * via btf_modifier_show() past the parent typedef to the child
> +	 * struct, int etc it is defined as.  In such cases, the type_id
> +	 * still represents the starting type while the the struct btf_type *
> +	 * in our show->state points at the resolved type of the typedef.
> +	 */
> +	t = btf_type_by_id(show->btf, id);
> +	if (!t)
> +		return show->state.type_name;
> +
> +	/*
> +	 * 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);
> +			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)
> +				return show->state.type_name;
> +			if (!t)
> +				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);
> +		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"?

> +	/* 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;
> +		break;
> +	}
> +
> +	/* pointer does not require parens */
> +	if (kinds & BTF_KIND_BIT(BTF_KIND_PTR))
> +		parens = "";
> +	/* typedef does not require struct/union/enum prefix */
> +	if (kinds & BTF_KIND_BIT(BTF_KIND_TYPEDEF))
> +		prefix = "";
> +
> +	if (!type_name || strlen(type_name) == 0) {
> +		if (allow_anon)
> +			type_name = "";
> +		else
> +			return show->state.type_name;
> +	}
> +
> +	/* Even if we don't want type name info, we want parentheses etc */
> +	if (show->flags & BTF_SHOW_NONAME)
> +		snprintf(show->state.type_name, sizeof(show->state.type_name),
> +			 "%s", parens);
> +	else
> +		snprintf(show->state.type_name, sizeof(show->state.type_name),
> +			 "(%s%s%s%s%s%s)%s",
> +			 prefix,
> +			 strlen(prefix) > 0 && strlen(type_name) > 0 ? " " : "",
> +			 type_name,
> +			 strlen(ptr_suffix) > 0 ? " " : "", ptr_suffix,
> +			 array_suffix, parens);
> +
> +	return show->state.type_name;
> +}
> +
> +static inline const char *btf_show_name(struct btf_show *show)
> +{
> +	const struct btf_type *t = show->state.type;
> +	const struct btf_member *m = show->state.member;
> +	const char *member = NULL;
> +	const char *type = "";
> +
> +	show->state.name[0] = '\0';
> +
> +	if ((!m && !t) || show->state.array_member)
> +		return show->state.name;
> +
> +	if (m)
> +		t = btf_type_skip_qualifiers(show->btf, m->type);
> +
> +	if (t) {
> +		type = btf_show_type_name(show, t);
> +		if (!type)
> +			return show->state.name;
> +	}
> +
> +	if (m && !(show->flags & BTF_SHOW_NONAME)) {
> +		member = btf_name_by_offset(show->btf, m->name_off);
> +		if (member && strlen(member) > 0) {
> +			snprintf(show->state.name, sizeof(show->state.name),
> +				 ".%s = %s", member, type);
> +			return show->state.name;
> +		}
> +	}
> +
> +	snprintf(show->state.name, sizeof(show->state.name), "%s", type);
> +
> +	return show->state.name;
> +}
> +
> +#define btf_show(show, ...)						      \
> +	do {								      \
> +		if (!show->state.depth_check)				      \

As I mentioned above, some comments will be good to understand here.

> +			show->showfn(show, __VA_ARGS__);		      \
> +	} while (0)
> +
> +static inline const char *__btf_show_indent(struct btf_show *show)
> +{
> +	const char *indents = "                                ";
> +	const char *indent = &indents[strlen(indents)];
> +
> +	if ((indent - show->state.depth) >= indents)
> +		return indent - show->state.depth;
> +	return indents;
> +}
> +
> +#define btf_show_indent(show)						       \
> +	((show->flags & BTF_SHOW_COMPACT) ? "" : __btf_show_indent(show))
> +
> +#define btf_show_newline(show)						       \
> +	((show->flags & BTF_SHOW_COMPACT) ? "" : "\n")
> +
> +#define btf_show_delim(show)						       \
> +	(show->state.depth == 0 ? "" :					       \
> +	 ((show->flags & BTF_SHOW_COMPACT) && show->state.type &&	       \
> +	  BTF_INFO_KIND(show->state.type->info) == BTF_KIND_UNION) ? "|" : ",")
> +
> +#define btf_show_type_value(show, fmt, value)				       \
> +	do {								       \
> +		if ((value) != 0 || (show->flags & BTF_SHOW_ZERO) ||	       \
> +		    show->state.depth == 0) {				       \
> +			btf_show(show, "%s%s" fmt "%s%s",		       \
> +				 btf_show_indent(show),			       \
> +				 btf_show_name(show),			       \
> +				 value, btf_show_delim(show),		       \
> +				 btf_show_newline(show));		       \
> +			if (show->state.depth > show->state.depth_shown)       \
> +				show->state.depth_shown = show->state.depth;   \
> +		}							       \
> +	} while (0)
> +
> +#define btf_show_type_values(show, fmt, ...)				       \
> +	do {								       \
> +		btf_show(show, "%s%s" fmt "%s%s", btf_show_indent(show),       \
> +			 btf_show_name(show),				       \
> +			 __VA_ARGS__, btf_show_delim(show),		       \
> +			 btf_show_newline(show));			       \
> +		if (show->state.depth > show->state.depth_shown)	       \
> +			show->state.depth_shown = show->state.depth;	       \
> +	} while (0)
> +
[...]
>   
>   static int btf_array_check_member(struct btf_verifier_env *env,
> @@ -2104,28 +2489,87 @@ static void btf_array_log(struct btf_verifier_env *env,
>   			 array->type, array->index_type, array->nelems);
>   }
>   
> -static void btf_array_seq_show(const struct btf *btf, const struct btf_type *t,
> -			       u32 type_id, void *data, u8 bits_offset,
> -			       struct seq_file *m)
> +static void __btf_array_show(const struct btf *btf, const struct btf_type *t,
> +			     u32 type_id, void *data, u8 bits_offset,
> +			     struct btf_show *show)
>   {
>   	const struct btf_array *array = btf_type_array(t);
>   	const struct btf_kind_operations *elem_ops;
>   	const struct btf_type *elem_type;
> -	u32 i, elem_size, elem_type_id;
> +	u32 i, elem_size = 0, elem_type_id;
> +	u16 encoding = 0;
>   
>   	elem_type_id = array->type;
> -	elem_type = btf_type_id_size(btf, &elem_type_id, &elem_size);
> +	elem_type = btf_type_skip_modifiers(btf, elem_type_id, NULL);
> +	if (elem_type && btf_type_has_size(elem_type))
> +		elem_size = elem_type->size;
> +
> +	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?

> +	}
> +
> +	btf_show_start_array_type(show, t, type_id, encoding);
> +
> +	if (!elem_type)
> +		goto out;
>   	elem_ops = btf_type_ops(elem_type);
> -	seq_puts(m, "[");
> +
>   	for (i = 0; i < array->nelems; i++) {
> -		if (i)
> -			seq_puts(m, ",");
>   
> -		elem_ops->seq_show(btf, elem_type, elem_type_id, data,
> -				   bits_offset, m);
> +		btf_show_start_array_member(show);
> +
> +		elem_ops->show(btf, elem_type, elem_type_id, data,
> +			       bits_offset, show);
>   		data += elem_size;
> +
> +		btf_show_end_array_member(show);
> +
> +		if (show->state.array_terminated)
> +			break;
>   	}
> -	seq_puts(m, "]");
> +out:
> +	btf_show_end_array_type(show);
> +}
> +
[...]