[PATCH v2 bpf-next 0/7] bpf, printk: add BTF-based type printing

[PATCH v2 bpf-next 0/7] bpf, printk: add BTF-based type printing

[Alan Maguire]

The printk family of functions support printing specific pointer types
using %p format specifiers (MAC addresses, IP addresses, etc).  For
full details see Documentation/core-api/printk-formats.rst.

This patchset proposes introducing a "print typed pointer" format
specifier "%pT"; the argument associated with the specifier is of
form "struct btf_ptr *" which consists of a .ptr value and a .type
value specifying a stringified type (e.g. "struct sk_buff") or
an .id value specifying a BPF Type Format (BTF) id identifying
the appropriate type it points to.

There is already support in kernel/bpf/btf.c for "show" functionality;
the changes here generalize that support from seq-file specific
verifier display to the more generic case and add another specific
use case; snprintf()-style rendering of type information to a
provided buffer.  This support is then used to support printk
rendering of types, but the intent is to provide a function
that might be useful in other in-kernel scenarios; for example:

- ftrace could possibly utilize the function to support typed
  display of function arguments by cross-referencing BTF function
  information to derive the types of arguments
- oops/panic messaging could extend the information displayed to
  dig into data structures associated with failing functions

The above potential use cases hint at a potential reply to
a reasonable objection that such typed display should be
solved by tracing programs, where the in kernel tracing records
data and the userspace program prints it out.  While this
is certainly the recommended approach for most cases, I
believe having an in-kernel mechanism would be valuable
also.

The function the printk() family of functions rely on
could potentially be used directly for other use cases
like ftrace where we might have the BTF ids of the
pointers we wish to display; its signature is as follows:

int btf_type_snprintf_show(const struct btf *btf, u32 type_id, void *obj,
                           char *buf, int len, u64 flags);

So if ftrace say had the BTF ids of the types of arguments,
we see that the above would allow us to convert the
pointer data into displayable form.

To give a flavour for what the printed-out data looks like,
here we use pr_info() to display a struct sk_buff *.

  struct sk_buff *skb = alloc_skb(64, GFP_KERNEL);

  pr_info("%pT", BTF_PTR_TYPE(skb, "struct sk_buff"));

...gives us:

(struct sk_buff){
 .transport_header = (__u16)65535,
 .mac_header = (__u16)65535,
 .end = (sk_buff_data_t)192,
 .head = (unsigned char *)000000007524fd8b,
 .data = (unsigned char *)000000007524fd8b,
 .truesize = (unsigned int)768,
 .users = (refcount_t){
  .refs = (atomic_t){
   .counter = (int)1,
  },
 },
}

For bpf_trace_printk() a "struct __btf_ptr *" is used as
argument; see tools/testing/selftests/bpf/progs/netif_receive_skb.c
for example usage.

The hope is that this functionality will be useful for debugging,
and possibly help facilitate the cases mentioned above in the future.

Changes since v1:

- changed format to be more drgn-like, rendering indented type info
  along with type names by default (Alexei)
- zeroed values are omitted (Arnaldo) by default unless the '0'
  modifier is specified (Alexei)
- added an option to print pointer values without obfuscation.
  The reason to do this is the sysctls controlling pointer display
  are likely to be irrelevant in many if not most tracing contexts.
  Some questions on this in the outstanding questions section below...
- reworked printk format specifer so that we no longer rely on format
  %pT<type> but instead use a struct * which contains type information
  (Rasmus). This simplifies the printk parsing, makes use more dynamic
  and also allows specification by BTF id as well as name.
- ensured that BTF-specific printk code is bracketed by
  #if ENABLED(CONFIG_BTF_PRINTF)
- removed incorrect patch which tried to fix dereferencing of resolved
  BTF info for vmlinux; instead we skip modifiers for the relevant
  case (array element type/size determination) (Alexei).
- fixed issues with negative snprintf format length (Rasmus)
- added test cases for various data structure formats; base types,
  typedefs, structs, etc.
- tests now iterate through all typedef, enum, struct and unions
  defined for vmlinux BTF and render a version of the target dummy
  value which is either all zeros or all 0xff values; the idea is this
  exercises the "skip if zero" and "print everything" cases.
- added support in BPF for using the %pT format specifier in
  bpf_trace_printk()
- added BPF tests which ensure %pT format specifier use works (Alexei).

Outstanding issues

- currently %pT is not allowed in BPF programs when lockdown is active
  prohibiting BPF_READ; is that sufficient?
- do we want to further restrict the non-obfuscated pointer format
  specifier %pTx; for example blocking unprivileged BPF programs from
  using it?
- likely still need a better answer for vmlinux BTF initialization
  than the current approach taken; early boot initialization is one
  way to go here.
- may be useful to have a "print integers as hex" format modifier (Joe)

Important note: if running test_printf.ko - the version in the bpf-next
tree will induce a panic when running the fwnode_pointer() tests due
to a kobject issue; applying the patch in

https://lkml.org/lkml/2020/4/17/389

...resolved this issue for me.

Alan Maguire (7):
  bpf: provide function to get vmlinux BTF information
  bpf: move to generic BTF show support, apply it to seq files/strings
  checkpatch: add new BTF pointer format specifier
  printk: add type-printing %pT format specifier which uses BTF
  printk: extend test_printf to test %pT BTF-based format specifier
  bpf: add support for %pT format specifier for bpf_trace_printk()
    helper
  bpf: add tests for %pT format specifier

 Documentation/core-api/printk-formats.rst          |  15 +
 include/linux/bpf.h                                |   2 +
 include/linux/btf.h                                |  46 +-
 include/linux/printk.h                             |  16 +
 include/uapi/linux/bpf.h                           |  27 +-
 kernel/bpf/btf.c                                   | 794 ++++++++++++++++++---
 kernel/bpf/verifier.c                              |  18 +-
 kernel/trace/bpf_trace.c                           |  21 +-
 lib/Kconfig                                        |  16 +
 lib/test_printf.c                                  | 301 ++++++++
 lib/vsprintf.c                                     | 113 +++
 scripts/checkpatch.pl                              |   2 +-
 tools/include/uapi/linux/bpf.h                     |  27 +-
 .../selftests/bpf/prog_tests/trace_printk_btf.c    |  83 +++
 .../selftests/bpf/progs/netif_receive_skb.c        |  81 +++
 15 files changed, 1439 insertions(+), 123 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/trace_printk_btf.c
 create mode 100644 tools/testing/selftests/bpf/progs/netif_receive_skb.c

-- 
1.8.3.1

[PATCH v2 bpf-next 1/7] bpf: provide function to get vmlinux BTF information

[Alan Maguire]

It will be used later for BTF printk() support

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 include/linux/bpf.h   |  2 ++
 kernel/bpf/verifier.c | 18 ++++++++++++------
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index cf4b6e4..de19a35 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1203,6 +1203,8 @@ int bpf_check(struct bpf_prog **fp, union bpf_attr *attr,
 	      union bpf_attr __user *uattr);
 void bpf_patch_call_args(struct bpf_insn *insn, u32 stack_depth);
 
+struct btf *bpf_get_btf_vmlinux(void);
+
 /* Map specifics */
 struct xdp_buff;
 struct sk_buff;
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 2a1826c..c6b1ba0 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -10728,6 +10728,17 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
 	}
 }
 
+struct btf *bpf_get_btf_vmlinux(void)
+{
+	if (!btf_vmlinux && IS_ENABLED(CONFIG_DEBUG_INFO_BTF)) {
+		mutex_lock(&bpf_verifier_lock);
+		if (!btf_vmlinux)
+			btf_vmlinux = btf_parse_vmlinux();
+		mutex_unlock(&bpf_verifier_lock);
+	}
+	return btf_vmlinux;
+}
+
 int bpf_check(struct bpf_prog **prog, union bpf_attr *attr,
 	      union bpf_attr __user *uattr)
 {
@@ -10761,12 +10772,7 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr,
 	env->ops = bpf_verifier_ops[env->prog->type];
 	is_priv = capable(CAP_SYS_ADMIN);
 
-	if (!btf_vmlinux && IS_ENABLED(CONFIG_DEBUG_INFO_BTF)) {
-		mutex_lock(&bpf_verifier_lock);
-		if (!btf_vmlinux)
-			btf_vmlinux = btf_parse_vmlinux();
-		mutex_unlock(&bpf_verifier_lock);
-	}
+	bpf_get_btf_vmlinux();
 
 	/* grab the mutex to protect few globals used by verifier */
 	if (!is_priv)
-- 
1.8.3.1

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

[Alan Maguire]

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;
+		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 */
+	} 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 = "[][][][][][][][][][]";
+	const char *array_suffix = &array_suffixes[strlen(array_suffixes)];
+	const char *ptr_suffixes = "**********";
+	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;
+	}
+	/* 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)				      \
+			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 inline void btf_show_start_type(struct btf_show *show,
+				       const struct btf_type *t,
+				       u32 type_id)
+{
+	show->state.type = t;
+	show->state.type_id = type_id;
+	show->state.name[0] = '\0';
+}
+
+static inline void btf_show_end_type(struct btf_show *show)
+{
+	show->state.type = NULL;
+	show->state.type_id = 0;
+	show->state.name[0] = '\0';
+}
+
+static inline void btf_show_start_aggr_type(struct btf_show *show,
+					    const struct btf_type *t,
+					    u32 type_id)
+{
+	btf_show_start_type(show, t, type_id);
+	btf_show(show, "%s%s%s", btf_show_indent(show),
+		 btf_show_name(show),
+		 btf_show_newline(show));
+	show->state.depth++;
+}
+
+static inline void btf_show_end_aggr_type(struct btf_show *show,
+					  const char *suffix)
+{
+	show->state.depth--;
+	btf_show(show, "%s%s%s%s", btf_show_indent(show), suffix,
+		 btf_show_delim(show), btf_show_newline(show));
+	btf_show_end_type(show);
+}
+
+static inline void btf_show_start_member(struct btf_show *show,
+					 const struct btf_member *m)
+{
+	show->state.member = m;
+}
+
+static inline void btf_show_start_array_member(struct btf_show *show)
+{
+	show->state.array_member = 1;
+	btf_show_start_member(show, NULL);
+}
+
+static inline void btf_show_end_member(struct btf_show *show)
+{
+	show->state.member = NULL;
+}
+
+static inline void btf_show_end_array_member(struct btf_show *show)
+{
+	show->state.array_member = 0;
+	btf_show_end_member(show);
+}
+
+static inline void btf_show_start_array_type(struct btf_show *show,
+					     const struct btf_type *t,
+					     u32 type_id,
+					     u16 array_encoding)
+{
+	show->state.array_encoding = array_encoding;
+	show->state.array_terminated = 0;
+	btf_show_start_aggr_type(show, t, type_id);
+}
+
+static inline void btf_show_end_array_type(struct btf_show *show)
+{
+	show->state.array_encoding = 0;
+	show->state.array_terminated = 0;
+	btf_show_end_aggr_type(show, "]");
+}
+
+static inline void btf_show_start_struct_type(struct btf_show *show,
+					      const struct btf_type *t,
+					      u32 type_id)
+{
+	btf_show_start_aggr_type(show, t, type_id);
+}
+
+static inline void btf_show_end_struct_type(struct btf_show *show)
+{
+	btf_show_end_aggr_type(show, "}");
+}
+
 __printf(2, 3) static void __btf_verifier_log(struct bpf_verifier_log *log,
 					      const char *fmt, ...)
 {
@@ -1249,11 +1609,11 @@ static int btf_df_resolve(struct btf_verifier_env *env,
 	return -EINVAL;
 }
 
-static void btf_df_seq_show(const struct btf *btf, const struct btf_type *t,
-			    u32 type_id, void *data, u8 bits_offsets,
-			    struct seq_file *m)
+static void btf_df_show(const struct btf *btf, const struct btf_type *t,
+			u32 type_id, void *data, u8 bits_offsets,
+			struct btf_show *show)
 {
-	seq_printf(m, "<unsupported kind:%u>", BTF_INFO_KIND(t->info));
+	btf_show(show, "<unsupported kind:%u>", BTF_INFO_KIND(t->info));
 }
 
 static int btf_int_check_member(struct btf_verifier_env *env,
@@ -1426,7 +1786,7 @@ static void btf_int_log(struct btf_verifier_env *env,
 			 btf_int_encoding_str(BTF_INT_ENCODING(int_data)));
 }
 
-static void btf_int128_print(struct seq_file *m, void *data)
+static void btf_int128_print(struct btf_show *show, void *data)
 {
 	/* data points to a __int128 number.
 	 * Suppose
@@ -1445,9 +1805,10 @@ static void btf_int128_print(struct seq_file *m, void *data)
 	lower_num = *(u64 *)data;
 #endif
 	if (upper_num == 0)
-		seq_printf(m, "0x%llx", lower_num);
+		btf_show_type_value(show, "0x%llx", lower_num);
 	else
-		seq_printf(m, "0x%llx%016llx", upper_num, lower_num);
+		btf_show_type_values(show, "0x%llx%016llx", upper_num,
+				     lower_num);
 }
 
 static void btf_int128_shift(u64 *print_num, u16 left_shift_bits,
@@ -1491,8 +1852,8 @@ static void btf_int128_shift(u64 *print_num, u16 left_shift_bits,
 #endif
 }
 
-static void btf_bitfield_seq_show(void *data, u8 bits_offset,
-				  u8 nr_bits, struct seq_file *m)
+static void btf_bitfield_show(void *data, u8 bits_offset,
+			      u8 nr_bits, struct btf_show *show)
 {
 	u16 left_shift_bits, right_shift_bits;
 	u8 nr_copy_bytes;
@@ -1512,14 +1873,14 @@ static void btf_bitfield_seq_show(void *data, u8 bits_offset,
 	right_shift_bits = BITS_PER_U128 - nr_bits;
 
 	btf_int128_shift(print_num, left_shift_bits, right_shift_bits);
-	btf_int128_print(m, print_num);
+	btf_int128_print(show, print_num);
 }
 
 
-static void btf_int_bits_seq_show(const struct btf *btf,
-				  const struct btf_type *t,
-				  void *data, u8 bits_offset,
-				  struct seq_file *m)
+static void btf_int_bits_show(const struct btf *btf,
+			      const struct btf_type *t,
+			      void *data, u8 bits_offset,
+			      struct btf_show *show)
 {
 	u32 int_data = btf_type_int(t);
 	u8 nr_bits = BTF_INT_BITS(int_data);
@@ -1532,55 +1893,74 @@ static void btf_int_bits_seq_show(const struct btf *btf,
 	total_bits_offset = bits_offset + BTF_INT_OFFSET(int_data);
 	data += BITS_ROUNDDOWN_BYTES(total_bits_offset);
 	bits_offset = BITS_PER_BYTE_MASKED(total_bits_offset);
-	btf_bitfield_seq_show(data, bits_offset, nr_bits, m);
+	btf_bitfield_show(data, bits_offset, nr_bits, show);
 }
 
-static void btf_int_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_int_show(const struct btf *btf, const struct btf_type *t,
+			 u32 type_id, void *data, u8 bits_offset,
+			 struct btf_show *show)
 {
 	u32 int_data = btf_type_int(t);
 	u8 encoding = BTF_INT_ENCODING(int_data);
 	bool sign = encoding & BTF_INT_SIGNED;
 	u8 nr_bits = BTF_INT_BITS(int_data);
 
+	btf_show_start_type(show, t, type_id);
+
 	if (bits_offset || BTF_INT_OFFSET(int_data) ||
 	    BITS_PER_BYTE_MASKED(nr_bits)) {
-		btf_int_bits_seq_show(btf, t, data, bits_offset, m);
-		return;
+		btf_int_bits_show(btf, t, data, bits_offset, show);
+		goto out;
 	}
 
 	switch (nr_bits) {
 	case 128:
-		btf_int128_print(m, data);
+		btf_int128_print(show, data);
 		break;
 	case 64:
 		if (sign)
-			seq_printf(m, "%lld", *(s64 *)data);
+			btf_show_type_value(show, "%lld", *(s64 *)data);
 		else
-			seq_printf(m, "%llu", *(u64 *)data);
+			btf_show_type_value(show, "%llu", *(u64 *)data);
 		break;
 	case 32:
 		if (sign)
-			seq_printf(m, "%d", *(s32 *)data);
+			btf_show_type_value(show, "%d", *(s32 *)data);
 		else
-			seq_printf(m, "%u", *(u32 *)data);
+			btf_show_type_value(show, "%u", *(u32 *)data);
 		break;
 	case 16:
 		if (sign)
-			seq_printf(m, "%d", *(s16 *)data);
+			btf_show_type_value(show, "%d", *(s16 *)data);
 		else
-			seq_printf(m, "%u", *(u16 *)data);
+			btf_show_type_value(show, "%u", *(u16 *)data);
 		break;
 	case 8:
+		if (show->state.array_encoding == BTF_INT_CHAR) {
+			/* check for null terminator */
+			if (show->state.array_terminated)
+				break;
+			if (*(char *)data == '\0') {
+				show->state.array_terminated = 1;
+				break;
+			}
+			if (isprint(*(char *)data)) {
+				btf_show_type_value(show, "'%c'",
+						    *(char *)data);
+				break;
+			}
+		}
 		if (sign)
-			seq_printf(m, "%d", *(s8 *)data);
+			btf_show_type_value(show, "%d", *(s8 *)data);
 		else
-			seq_printf(m, "%u", *(u8 *)data);
+			btf_show_type_value(show, "%u", *(u8 *)data);
 		break;
 	default:
-		btf_int_bits_seq_show(btf, t, data, bits_offset, m);
+		btf_int_bits_show(btf, t, data, bits_offset, show);
+		break;
 	}
+out:
+	btf_show_end_type(show);
 }
 
 static const struct btf_kind_operations int_ops = {
@@ -1589,7 +1969,7 @@ static void btf_int_seq_show(const struct btf *btf, const struct btf_type *t,
 	.check_member = btf_int_check_member,
 	.check_kflag_member = btf_int_check_kflag_member,
 	.log_details = btf_int_log,
-	.seq_show = btf_int_seq_show,
+	.show = btf_int_show,
 };
 
 static int btf_modifier_check_member(struct btf_verifier_env *env,
@@ -1853,34 +2233,39 @@ static int btf_ptr_resolve(struct btf_verifier_env *env,
 	return 0;
 }
 
-static void btf_modifier_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_modifier_show(const struct btf *btf,
+			      const struct btf_type *t,
+			      u32 type_id, void *data,
+			      u8 bits_offset, struct btf_show *show)
 {
 	if (btf->resolved_ids)
 		t = btf_type_id_resolve(btf, &type_id);
 	else
 		t = btf_type_skip_modifiers(btf, type_id, NULL);
 
-	btf_type_ops(t)->seq_show(btf, t, type_id, data, bits_offset, m);
+	btf_type_ops(t)->show(btf, t, type_id, data, bits_offset, show);
 }
 
-static void btf_var_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_var_show(const struct btf *btf, const struct btf_type *t,
+			 u32 type_id, void *data, u8 bits_offset,
+			 struct btf_show *show)
 {
 	t = btf_type_id_resolve(btf, &type_id);
 
-	btf_type_ops(t)->seq_show(btf, t, type_id, data, bits_offset, m);
+	btf_type_ops(t)->show(btf, t, type_id, data, bits_offset, show);
 }
 
-static void btf_ptr_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_ptr_show(const struct btf *btf, const struct btf_type *t,
+			 u32 type_id, void *data, u8 bits_offset,
+			 struct btf_show *show)
 {
-	/* It is a hashed value */
-	seq_printf(m, "%p", *(void **)data);
+	btf_show_start_type(show, t, type_id);
+	/* It is a hashed value unless BTF_SHOW_PTR_RAW is specified */
+	if (show->flags & BTF_SHOW_PTR_RAW)
+		btf_show_type_value(show, "%px", *(void **)data);
+	else
+		btf_show_type_value(show, "%p", *(void **)data);
+	btf_show_end_type(show);
 }
 
 static void btf_ref_type_log(struct btf_verifier_env *env,
@@ -1895,7 +2280,7 @@ static void btf_ref_type_log(struct btf_verifier_env *env,
 	.check_member = btf_modifier_check_member,
 	.check_kflag_member = btf_modifier_check_kflag_member,
 	.log_details = btf_ref_type_log,
-	.seq_show = btf_modifier_seq_show,
+	.show = btf_modifier_show,
 };
 
 static struct btf_kind_operations ptr_ops = {
@@ -1904,7 +2289,7 @@ static void btf_ref_type_log(struct btf_verifier_env *env,
 	.check_member = btf_ptr_check_member,
 	.check_kflag_member = btf_generic_check_kflag_member,
 	.log_details = btf_ref_type_log,
-	.seq_show = btf_ptr_seq_show,
+	.show = btf_ptr_show,
 };
 
 static s32 btf_fwd_check_meta(struct btf_verifier_env *env,
@@ -1945,7 +2330,7 @@ static void btf_fwd_type_log(struct btf_verifier_env *env,
 	.check_member = btf_df_check_member,
 	.check_kflag_member = btf_df_check_kflag_member,
 	.log_details = btf_fwd_type_log,
-	.seq_show = btf_df_seq_show,
+	.show = btf_df_show,
 };
 
 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;
+	}
+
+	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);
+}
+
+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_member *m = show->state.member;
+
+	/*
+	 * First check if any members would be shown (are non-zero).
+	 */
+	if (show->state.depth > 0 && !(show->flags & BTF_SHOW_ZERO)) {
+		if (!show->state.depth_check) {
+			show->state.depth_check = show->state.depth + 1;
+			show->state.depth_shown = 0;
+		}
+		__btf_array_show(btf, t, type_id, data, bits_offset, show);
+		show->state.member = m;
+
+		if (show->state.depth_check != show->state.depth + 1)
+			return;
+		show->state.depth_check = 0;
+
+		if (show->state.depth_shown <= show->state.depth)
+			return;
+		/*
+		 * Reaching here indicates we have recursed and found
+		 * non-zero array member(s).
+		 */
+	}
+	__btf_array_show(btf, t, type_id, data, bits_offset, show);
 }
 
 static struct btf_kind_operations array_ops = {
@@ -2134,7 +2578,7 @@ static void btf_array_seq_show(const struct btf *btf, const struct btf_type *t,
 	.check_member = btf_array_check_member,
 	.check_kflag_member = btf_generic_check_kflag_member,
 	.log_details = btf_array_log,
-	.seq_show = btf_array_seq_show,
+	.show = btf_array_show,
 };
 
 static int btf_struct_check_member(struct btf_verifier_env *env,
@@ -2357,15 +2801,15 @@ int btf_find_spin_lock(const struct btf *btf, const struct btf_type *t)
 	return off;
 }
 
-static void btf_struct_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_struct_show(const struct btf *btf, const struct btf_type *t,
+			      u32 type_id, void *data, u8 bits_offset,
+			      struct btf_show *show)
 {
-	const char *seq = BTF_INFO_KIND(t->info) == BTF_KIND_UNION ? "|" : ",";
 	const struct btf_member *member;
 	u32 i;
 
-	seq_puts(m, "{");
+	btf_show_start_struct_type(show, t, type_id);
+
 	for_each_member(i, t, member) {
 		const struct btf_type *member_type = btf_type_by_id(btf,
 								member->type);
@@ -2374,23 +2818,55 @@ static void btf_struct_seq_show(const struct btf *btf, const struct btf_type *t,
 		u32 bytes_offset;
 		u8 bits8_offset;
 
-		if (i)
-			seq_puts(m, seq);
+		btf_show_start_member(show, member);
 
 		member_offset = btf_member_bit_offset(t, member);
 		bitfield_size = btf_member_bitfield_size(t, member);
 		bytes_offset = BITS_ROUNDDOWN_BYTES(member_offset);
 		bits8_offset = BITS_PER_BYTE_MASKED(member_offset);
 		if (bitfield_size) {
-			btf_bitfield_seq_show(data + bytes_offset, bits8_offset,
-					      bitfield_size, m);
+			btf_bitfield_show(data + bytes_offset, bits8_offset,
+					  bitfield_size, show);
 		} else {
 			ops = btf_type_ops(member_type);
-			ops->seq_show(btf, member_type, member->type,
-				      data + bytes_offset, bits8_offset, m);
+			ops->show(btf, member_type, member->type,
+				  data + bytes_offset, bits8_offset, show);
 		}
+
+		btf_show_end_member(show);
 	}
-	seq_puts(m, "}");
+
+	btf_show_end_struct_type(show);
+}
+
+static void btf_struct_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_member *m = show->state.member;
+
+	/* First check if any members would be shown (are non-zero) */
+	if (show->state.depth > 0 && !(show->flags & BTF_SHOW_ZERO)) {
+		if (!show->state.depth_check) {
+			show->state.depth_check = show->state.depth + 1;
+			show->state.depth_shown = 0;
+		}
+		__btf_struct_show(btf, t, type_id, data, bits_offset, show);
+		/* Restore saved member data here */
+		show->state.member = m;
+		if (show->state.depth_check != show->state.depth + 1)
+			return;
+		show->state.depth_check = 0;
+
+		if (show->state.depth_shown <= show->state.depth)
+			return;
+		/*
+		 * Reaching here indicates we have recursed and found
+		 * non-zero child values.
+		 */
+	}
+
+	__btf_struct_show(btf, t, type_id, data, bits_offset, show);
 }
 
 static struct btf_kind_operations struct_ops = {
@@ -2399,7 +2875,7 @@ static void btf_struct_seq_show(const struct btf *btf, const struct btf_type *t,
 	.check_member = btf_struct_check_member,
 	.check_kflag_member = btf_generic_check_kflag_member,
 	.log_details = btf_struct_log,
-	.seq_show = btf_struct_seq_show,
+	.show = btf_struct_show,
 };
 
 static int btf_enum_check_member(struct btf_verifier_env *env,
@@ -2530,24 +3006,30 @@ static void btf_enum_log(struct btf_verifier_env *env,
 	btf_verifier_log(env, "size=%u vlen=%u", t->size, btf_type_vlen(t));
 }
 
-static void btf_enum_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_enum_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_enum *enums = btf_type_enum(t);
 	u32 i, nr_enums = btf_type_vlen(t);
 	int v = *(int *)data;
 
+	btf_show_start_type(show, t, type_id);
+
 	for (i = 0; i < nr_enums; i++) {
-		if (v == enums[i].val) {
-			seq_printf(m, "%s",
-				   __btf_name_by_offset(btf,
-							enums[i].name_off));
-			return;
-		}
+		if (v != enums[i].val)
+			continue;
+
+		btf_show_type_value(show, "%s",
+				    __btf_name_by_offset(btf,
+							 enums[i].name_off));
+
+		btf_show_end_type(show);
+		return;
 	}
 
-	seq_printf(m, "%d", v);
+	btf_show_type_value(show, "%d", v);
+	btf_show_end_type(show);
 }
 
 static struct btf_kind_operations enum_ops = {
@@ -2556,7 +3038,7 @@ static void btf_enum_seq_show(const struct btf *btf, const struct btf_type *t,
 	.check_member = btf_enum_check_member,
 	.check_kflag_member = btf_enum_check_kflag_member,
 	.log_details = btf_enum_log,
-	.seq_show = btf_enum_seq_show,
+	.show = btf_enum_show,
 };
 
 static s32 btf_func_proto_check_meta(struct btf_verifier_env *env,
@@ -2643,7 +3125,7 @@ static void btf_func_proto_log(struct btf_verifier_env *env,
 	.check_member = btf_df_check_member,
 	.check_kflag_member = btf_df_check_kflag_member,
 	.log_details = btf_func_proto_log,
-	.seq_show = btf_df_seq_show,
+	.show = btf_df_show,
 };
 
 static s32 btf_func_check_meta(struct btf_verifier_env *env,
@@ -2677,7 +3159,7 @@ static s32 btf_func_check_meta(struct btf_verifier_env *env,
 	.check_member = btf_df_check_member,
 	.check_kflag_member = btf_df_check_kflag_member,
 	.log_details = btf_ref_type_log,
-	.seq_show = btf_df_seq_show,
+	.show = btf_df_show,
 };
 
 static s32 btf_var_check_meta(struct btf_verifier_env *env,
@@ -2741,7 +3223,7 @@ static void btf_var_log(struct btf_verifier_env *env, const struct btf_type *t)
 	.check_member		= btf_df_check_member,
 	.check_kflag_member	= btf_df_check_kflag_member,
 	.log_details		= btf_var_log,
-	.seq_show		= btf_var_seq_show,
+	.show			= btf_var_show,
 };
 
 static s32 btf_datasec_check_meta(struct btf_verifier_env *env,
@@ -2867,24 +3349,26 @@ static void btf_datasec_log(struct btf_verifier_env *env,
 	btf_verifier_log(env, "size=%u vlen=%u", t->size, btf_type_vlen(t));
 }
 
-static void btf_datasec_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_datasec_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_var_secinfo *vsi;
 	const struct btf_type *var;
 	u32 i;
 
-	seq_printf(m, "section (\"%s\") = {", __btf_name_by_offset(btf, t->name_off));
+	btf_show_start_type(show, t, type_id);
+	btf_show_type_value(show, "section (\"%s\") = {",
+			    __btf_name_by_offset(btf, t->name_off));
 	for_each_vsi(i, t, vsi) {
 		var = btf_type_by_id(btf, vsi->type);
 		if (i)
-			seq_puts(m, ",");
-		btf_type_ops(var)->seq_show(btf, var, vsi->type,
-					    data + vsi->offset, bits_offset, m);
+			btf_show(show, ",");
+		btf_type_ops(var)->show(btf, var, vsi->type,
+					data + vsi->offset, bits_offset, show);
 	}
-	seq_puts(m, "}");
+	btf_show_end_type(show);
 }
 
 static const struct btf_kind_operations datasec_ops = {
@@ -2893,7 +3377,7 @@ static void btf_datasec_seq_show(const struct btf *btf,
 	.check_member		= btf_df_check_member,
 	.check_kflag_member	= btf_df_check_kflag_member,
 	.log_details		= btf_datasec_log,
-	.seq_show		= btf_datasec_seq_show,
+	.show			= btf_datasec_show,
 };
 
 static int btf_func_proto_check(struct btf_verifier_env *env,
@@ -4549,12 +5033,83 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog,
 	return 0;
 }
 
-void btf_type_seq_show(const struct btf *btf, u32 type_id, void *obj,
-		       struct seq_file *m)
+void btf_type_show(const struct btf *btf, u32 type_id, void *obj,
+		   struct btf_show *show)
 {
 	const struct btf_type *t = btf_type_by_id(btf, type_id);
 
-	btf_type_ops(t)->seq_show(btf, t, type_id, obj, 0, m);
+	show->btf = btf;
+	memset(&show->state, 0, sizeof(show->state));
+
+	btf_type_ops(t)->show(btf, t, type_id, obj, 0, show);
+}
+
+static void btf_seq_show(struct btf_show *show, const char *fmt, ...)
+{
+	va_list args;
+
+	va_start(args, fmt);
+	seq_vprintf((struct seq_file *)show->target, fmt, args);
+	va_end(args);
+}
+
+void btf_type_seq_show(const struct btf *btf, u32 type_id, void *obj,
+			struct seq_file *m)
+{
+	struct btf_show sseq;
+
+	sseq.target = m;
+	sseq.showfn = btf_seq_show;
+	sseq.flags = BTF_SHOW_NONAME | BTF_SHOW_COMPACT | BTF_SHOW_ZERO;
+
+	btf_type_show(btf, type_id, obj, &sseq);
+}
+
+struct btf_show_snprintf {
+	struct btf_show show;
+	int len_left;		/* space left in string */
+	int len;		/* length we would have written */
+};
+
+static void btf_snprintf_show(struct btf_show *show, const char *fmt, ...)
+{
+	struct btf_show_snprintf *ssnprintf = (struct btf_show_snprintf *)show;
+	va_list args;
+	int len;
+
+	va_start(args, fmt);
+	len = vsnprintf(show->target, ssnprintf->len_left, fmt, args);
+	va_end(args);
+
+	if (len < 0) {
+		ssnprintf->len_left = 0;
+		ssnprintf->len = len;
+	} else if (len > ssnprintf->len_left) {
+		/* no space, drive on to get length we would have written */
+		ssnprintf->len_left = 0;
+		ssnprintf->len += len;
+	} else {
+		ssnprintf->len_left -= len;
+		ssnprintf->len += len;
+		show->target += len;
+	}
+}
+
+int btf_type_snprintf_show(const struct btf *btf, u32 type_id, void *obj,
+			   char *buf, int len, u64 flags)
+{
+	struct btf_show_snprintf ssnprintf;
+
+	ssnprintf.show.target = buf;
+	ssnprintf.show.flags = flags;
+	ssnprintf.show.showfn = btf_snprintf_show;
+	ssnprintf.len_left = len;
+	ssnprintf.len = 0;
+
+	btf_type_show(btf, type_id, obj, (struct btf_show *)&ssnprintf);
+
+	/* Return length we would have written */
+	return ssnprintf.len;
 }
 
 #ifdef CONFIG_PROC_FS
-- 
1.8.3.1

[PATCH v2 bpf-next 3/7] checkpatch: add new BTF pointer format specifier

[Alan Maguire]

checkpatch complains about unknown format specifiers, so add
the BTF format specifier we will implement in a subsequent
patch to avoid errors.

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 scripts/checkpatch.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index eac40f0..8efbb23 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -6085,7 +6085,7 @@ sub process {
 					$specifier = $1;
 					$extension = $2;
 					$qualifier = $3;
-					if ($extension !~ /[SsBKRraEehMmIiUDdgVCbGNOxtf]/ ||
+					if ($extension !~ /[SsBKRraEehMmIiUDdgVCbGNOxtfT]/ ||
 					    ($extension eq "f" &&
 					     defined $qualifier && $qualifier !~ /^w/)) {
 						$bad_specifier = $specifier;
-- 
1.8.3.1

[PATCH v2 bpf-next 4/7] printk: add type-printing %pT format specifier which uses BTF

[Alan Maguire]

printk supports multiple pointer object type specifiers (printing
netdev features etc).  Extend this support using BTF to cover
arbitrary types.  "%pT" specifies the typed format, and the pointer
argument is a "struct btf_ptr *" where struct btf_ptr is as follows:

struct btf_ptr {
	void *ptr;
	const char *type;
	u32 id;
};

Either the "type" string ("struct sk_buff") or the BTF "id" can be
used to identify the type to use in displaying the associated "ptr"
value.  A convenience function to create and point at the struct
is provided:

	printk(KERN_INFO "%pT", BTF_PTR_TYPE(skb, struct sk_buff));

When invoked, BTF information is used to traverse the sk_buff *
and display it.  Support is present for structs, unions, enums,
typedefs and core types (though in the latter case there's not
much value in using this feature of course).

Default output is indented, but compact output can be specified
via the 'c' option.  Type names/member values can be suppressed
using the 'N' option.  Zero values are not displayed by default
but can be using the '0' option.  Pointer values are obfuscated
unless the 'x' option is specified.  As an example:

  struct sk_buff *skb = alloc_skb(64, GFP_KERNEL);
  pr_info("%pT", BTF_PTR_TYPE(skb, struct sk_buff));

...gives us:

(struct sk_buff){
 .transport_header = (__u16)65535,
 .mac_header = (__u16)65535,
 .end = (sk_buff_data_t)192,
 .head = (unsigned char *)000000006b71155a,
 .data = (unsigned char *)000000006b71155a,
 .truesize = (unsigned int)768,
 .users = (refcount_t){
  .refs = (atomic_t){
   .counter = (int)1,
  },
 },
 .extensions = (struct skb_ext *)00000000f486a130,
}

printk output is truncated at 1024 bytes.  For cases where overflow
is likely, the compact/no type names display modes may be used.

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 Documentation/core-api/printk-formats.rst |  15 ++++
 include/linux/btf.h                       |   3 +-
 include/linux/printk.h                    |  16 +++++
 lib/Kconfig                               |  16 +++++
 lib/vsprintf.c                            | 113 ++++++++++++++++++++++++++++++
 5 files changed, 162 insertions(+), 1 deletion(-)

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index 8ebe46b1..5c66097 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -545,6 +545,21 @@ For printing netdev_features_t.
 
 Passed by reference.
 
+BTF-based printing of pointer data
+----------------------------------
+If '%pT' is specified, use the struct btf_ptr * along with kernel vmlinux
+BPF Type Format (BTF) to show the typed data.  For example, specifying
+
+	printk(KERN_INFO "%pT", BTF_PTR_TYPE(skb, struct_sk_buff));
+
+will utilize BTF information to traverse the struct sk_buff * and display it.
+
+Supported modifers are
+ 'c' compact output (no indentation, newlines etc)
+ 'N' do not show type names
+ 'x' show raw pointers (no obfuscation)
+ '0' show zero-valued data (it is not shown by default)
+
 Thanks
 ======
 
diff --git a/include/linux/btf.h b/include/linux/btf.h
index d571125..7b585ab 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -169,10 +169,11 @@ static inline const struct btf_member *btf_type_member(const struct btf_type *t)
 	return (const struct btf_member *)(t + 1);
 }
 
+struct btf *btf_parse_vmlinux(void);
+
 #ifdef CONFIG_BPF_SYSCALL
 const struct btf_type *btf_type_by_id(const struct btf *btf, u32 type_id);
 const char *btf_name_by_offset(const struct btf *btf, u32 offset);
-struct btf *btf_parse_vmlinux(void);
 struct btf *bpf_prog_get_target_btf(const struct bpf_prog *prog);
 #else
 static inline const struct btf_type *btf_type_by_id(const struct btf *btf,
diff --git a/include/linux/printk.h b/include/linux/printk.h
index fcde0772..3c3ea53 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -528,4 +528,20 @@ static inline void print_hex_dump_debug(const char *prefix_str, int prefix_type,
 #define print_hex_dump_bytes(prefix_str, prefix_type, buf, len)	\
 	print_hex_dump_debug(prefix_str, prefix_type, 16, 1, buf, len, true)
 
+/**
+ * struct btf_ptr is used for %pT (typed pointer) display; the
+ * additional type string/BTF id are used to render the pointer
+ * data as the appropriate type.
+ */
+struct btf_ptr {
+	void *ptr;
+	const char *type;
+	u32 id;
+};
+
+#define	BTF_PTR_TYPE(ptrval, typeval) \
+	(&((struct btf_ptr){.ptr = ptrval, .type = #typeval}))
+
+#define BTF_PTR_ID(ptrval, idval) \
+	(&((struct btf_ptr){.ptr = ptrval, .id = idval}))
 #endif
diff --git a/lib/Kconfig b/lib/Kconfig
index 5d53f96..ac3a513 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -6,6 +6,22 @@
 config BINARY_PRINTF
 	def_bool n
 
+config BTF_PRINTF
+	bool "print type information using BPF type format"
+	depends on DEBUG_INFO_BTF
+	default n
+	help
+	  Print structures, unions etc pointed to by pointer argument using
+	  printk() family of functions (vsnprintf, printk, trace_printk, etc).
+	  For example, we can specify
+	  printk(KERN_INFO, "%pT<struct sk_buff>", skb); to print the skb
+	  data structure content, including all nested type data.
+	  Pointers within data structures displayed are not followed, and
+	  are obfuscated where specified in line with normal pointer display.
+	  via printk.
+
+	  Depends on availability of vmlinux BTF information.
+
 menu "Library routines"
 
 config RAID6_PQ
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 7c488a1..f9276f8 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -43,6 +43,7 @@
 #ifdef CONFIG_BLOCK
 #include <linux/blkdev.h>
 #endif
+#include <linux/btf.h>
 
 #include "../mm/internal.h"	/* For the trace_print_flags arrays */
 
@@ -2059,6 +2060,103 @@ char *fwnode_string(char *buf, char *end, struct fwnode_handle *fwnode,
 	return widen_string(buf, buf - buf_start, end, spec);
 }
 
+#if IS_ENABLED(CONFIG_BTF_PRINTF)
+#define btf_modifier_flag(c)	(c == 'c' ? BTF_SHOW_COMPACT :	\
+				 c == 'N' ? BTF_SHOW_NONAME :	\
+				 c == 'x' ? BTF_SHOW_PTR_RAW :	\
+				 c == '0' ? BTF_SHOW_ZERO : 0)
+
+static noinline_for_stack
+char *btf_string(char *buf, char *end, void *ptr, struct printf_spec spec,
+		 const char *fmt)
+{
+	struct btf_ptr *bp = (struct btf_ptr *)ptr;
+	u8 btf_kind = BTF_KIND_TYPEDEF;
+	const struct btf_type *t;
+	const struct btf *btf;
+	char *buf_start = buf;
+	const char *btf_type;
+	u64 flags = 0, mod;
+	s32 btf_id;
+
+	if (check_pointer(&buf, end, ptr, spec))
+		return buf;
+
+	if (check_pointer(&buf, end, bp->ptr, spec))
+		return buf;
+
+	while (isalnum(*fmt)) {
+		mod = btf_modifier_flag(*fmt);
+		if (!mod)
+			break;
+		flags |= mod;
+		fmt++;
+	}
+
+	btf = bpf_get_btf_vmlinux();
+	if (IS_ERR_OR_NULL(btf))
+		return ptr_to_id(buf, end, bp->ptr, spec);
+
+	if (bp->type != NULL) {
+		btf_type = bp->type;
+
+		if (strncmp(bp->type, "struct ", strlen("struct ")) == 0) {
+			btf_kind = BTF_KIND_STRUCT;
+			btf_type += strlen("struct ");
+		} else if (strncmp(btf_type, "union ", strlen("union ")) == 0) {
+			btf_kind = BTF_KIND_UNION;
+			btf_type += strlen("union ");
+		} else if (strncmp(btf_type, "enum ", strlen("enum ")) == 0) {
+			btf_kind = BTF_KIND_ENUM;
+			btf_type += strlen("enum ");
+		}
+
+		if (strlen(btf_type) == 0)
+			return ptr_to_id(buf, end, bp->ptr, spec);
+
+		/*
+		 * Assume type specified is a typedef as there's not much
+		 * benefit in specifying int types other than wasting time
+		 * on BTF lookups; we optimize for the most useful path.
+		 *
+		 * Fall back to BTF_KIND_INT if this fails.
+		 */
+		btf_id = btf_find_by_name_kind(btf, btf_type, btf_kind);
+		if (btf_id < 0)
+			btf_id = btf_find_by_name_kind(btf, btf_type,
+						       BTF_KIND_INT);
+	} else if (bp->id > 0)
+		btf_id = bp->id;
+	else
+		return ptr_to_id(buf, end, bp->ptr, spec);
+
+	if (btf_id > 0)
+		t = btf_type_by_id(btf, btf_id);
+	if (btf_id <= 0 || !t)
+		return ptr_to_id(buf, end, bp->ptr, spec);
+
+	buf += btf_type_snprintf_show(btf, btf_id, bp->ptr, buf,
+				      end - buf_start, flags);
+
+	return widen_string(buf, buf - buf_start, end, spec);
+}
+#else
+static noinline_for_stack
+char *btf_string(char *buf, char *end, void *ptr, struct printf_spec spec,
+	const char *fmt)
+{
+	struct btf_ptr *bp = (struct btf_ptr *)ptr;
+
+	if (check_pointer(&buf, end, ptr, spec))
+		return buf;
+
+	if (check_pointer(&buf, end, bp->ptr, spec))
+		return buf;
+
+	return ptr_to_id(buf, end, bp->ptr, spec);
+}
+#endif /* IS_ENABLED(CONFIG_BTF_PRINTF) */
+
 /*
  * Show a '%p' thing.  A kernel extension is that the '%p' is followed
  * by an extra set of alphanumeric characters that are extended format
@@ -2169,6 +2267,19 @@ char *fwnode_string(char *buf, char *end, struct fwnode_handle *fwnode,
  *		P node name, including a possible unit address
  * - 'x' For printing the address. Equivalent to "%lx".
  *
+ * - 'T[cNx0]' For printing struct btf_ptr * data using BPF Type Format (BTF).
+ *
+ *			Optional arguments are
+ *			c		compact (no indentation/newlines)
+ *			N		do not print type and member names
+ *			x		do not obfuscate pointers
+ *			0		show 0-valued data
+ *
+ *    BPF_PTR_TYPE(ptr, type) can be used to place pointer and type string
+ *    in the "struct btf_ptr *" expected; for example:
+ *
+ *	printk(KERN_INFO "%pT", BTF_PTR_TYPE(skb, struct sk_buff));
+ *
  * ** When making changes please also update:
  *	Documentation/core-api/printk-formats.rst
  *
@@ -2251,6 +2362,8 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 		if (!IS_ERR(ptr))
 			break;
 		return err_ptr(buf, end, ptr, spec);
+	case 'T':
+		return btf_string(buf, end, ptr, spec, fmt + 1);
 	}
 
 	/* default is to _not_ leak addresses, hash before printing */
-- 
1.8.3.1

[PATCH v2 bpf-next 5/7] printk: extend test_printf to test %pT BTF-based format specifier

[Alan Maguire]

Add tests to verify basic type display and to iterate through all
enums, structs, unions and typedefs ensuring expected behaviour
occurs.  Since test_printf can be built as a module we need to
export a BTF kind iterator function to allow us to iterate over
all names of a particular BTF kind.

These changes add up to approximately 20,000 new tests covering
all enum, struct, union and typedefs in vmlinux BTF.

Individual tests are also added for int, char, struct, enum
and typedefs which verify output is as expected.

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 include/linux/btf.h |  10 ++
 kernel/bpf/btf.c    |  35 ++++++
 lib/test_printf.c   | 301 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 346 insertions(+)

diff --git a/include/linux/btf.h b/include/linux/btf.h
index 7b585ab..7fa8926 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -188,4 +188,14 @@ static inline const char *btf_name_by_offset(const struct btf *btf,
 }
 #endif
 
+/* Following function used for testing BTF-based printk-family support */
+#ifdef CONFIG_BTF_PRINTF
+const char *btf_vmlinux_next_type_name(u8 kind, s32 *id);
+#else
+static inline const char *btf_vmlinux_next_type_name(u8 kind, s32 *id)
+{
+	return NULL;
+}
+#endif /* CONFIG_BTF_PRINTF */
+
 #endif
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index edf6455..99471dc 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -5249,3 +5249,38 @@ u32 btf_id(const struct btf *btf)
 {
 	return btf->id;
 }
+
+#ifdef CONFIG_BTF_PRINTF
+/*
+ * btf_vmlinux_next_type_name():  used in test_printf.c to
+ * iterate over types for testing.
+ * Exported as test_printf can be built as a module.
+ *
+ * @kind: BTF_KIND_* value
+ * @id: pointer to last id; value/result argument. When next
+ *      type name is found, we set *id to associated id.
+ * Returns:
+ *	Next type name, sets *id to associated id.
+ */
+const char *btf_vmlinux_next_type_name(u8 kind, s32 *id)
+{
+	const struct btf *btf = bpf_get_btf_vmlinux();
+	const struct btf_type *t;
+	const char *name;
+
+	if (!btf || !id)
+		return NULL;
+
+	for ((*id)++; *id <= btf->nr_types; (*id)++) {
+		t = btf->types[*id];
+		if (BTF_INFO_KIND(t->info) != kind)
+			continue;
+		name = btf_name_by_offset(btf, t->name_off);
+		if (name && strlen(name) > 0)
+			return name;
+	}
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(btf_vmlinux_next_type_name);
+#endif /* CONFIG_BTF_PRINTF */
diff --git a/lib/test_printf.c b/lib/test_printf.c
index 2d9f520..d37a3a2 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -23,6 +23,9 @@
 #include <linux/mm.h>
 
 #include <linux/property.h>
+#include <linux/bpf.h>
+#include <linux/btf.h>
+#include <linux/skbuff.h>
 
 #include "../tools/testing/selftests/kselftest_module.h"
 
@@ -644,6 +647,303 @@ static void __init fwnode_pointer(void)
 #endif
 }
 
+#define	__TEST_BTF(fmt, type, ptr, expected)				       \
+	test(expected, "%pT"fmt, ptr)
+
+#define TEST_BTF_C(type, var, ...)					       \
+	do {								       \
+		type var = __VA_ARGS__;					       \
+		struct btf_ptr *ptr = BTF_PTR_TYPE(&var, type);		       \
+		pr_debug("type %s: %pTc", #type, ptr);			       \
+		__TEST_BTF("c", type, ptr, "(" #type ")" #__VA_ARGS__);	       \
+	} while (0)
+
+#define TEST_BTF(fmt, type, var, expected, ...)				       \
+	do {								       \
+		type var = __VA_ARGS__;					       \
+		struct btf_ptr *ptr = BTF_PTR_TYPE(&var, type);		       \
+		pr_debug("type %s: %pT"fmt, #type, ptr);		       \
+		__TEST_BTF(fmt, type, ptr, expected);			       \
+	} while (0)
+
+#define	BTF_MAX_DATA_SIZE	65536
+
+static void __init
+btf_print_kind(u8 kind, const char *kind_name, u8 fillval)
+{
+	const char *fmt1 = "%pT", *fmt2 = "%pTN", *fmt3 = "%pT0";
+	const char *name, *fmt = fmt1;
+	int res1, res2, res3, res4;
+	char type_name[256];
+	u8 *dummy_data;
+	s32 id = 0;
+	char *buf;
+
+	dummy_data = kzalloc(BTF_MAX_DATA_SIZE, GFP_KERNEL);
+
+	/* fill our dummy data with supplied fillval. */
+	memset(dummy_data, fillval, BTF_MAX_DATA_SIZE);
+
+	buf = kzalloc(BTF_MAX_DATA_SIZE, GFP_KERNEL);
+
+	for (;;) {
+		name = btf_vmlinux_next_type_name(kind, &id);
+		if (!name)
+			break;
+
+		total_tests++;
+
+		snprintf(type_name, sizeof(type_name), "%s%s",
+			 kind_name, name);
+
+		res1 = snprintf(buf, BTF_MAX_DATA_SIZE, fmt1,
+				BTF_PTR_TYPE(dummy_data, type_name));
+		res2 = snprintf(buf, 0, fmt1,
+				BTF_PTR_TYPE(dummy_data, type_name));
+		res3 = snprintf(buf, BTF_MAX_DATA_SIZE, fmt2,
+				BTF_PTR_TYPE(dummy_data, type_name));
+		res4 = snprintf(buf, BTF_MAX_DATA_SIZE, fmt3,
+				BTF_PTR_TYPE(dummy_data, type_name));
+
+		/*
+		 * Ensure return value is > 0 and identical irrespective
+		 * of whether we pass in a big enough buffer;
+		 * also ensure that printing names always results in as
+		 * long/longer buffer length.
+		 */
+		if (res1 <= 0 || res2 <= 0 || res3 <= 0 || res4 <= 0) {
+			if (res3 <= 0)
+				fmt = fmt2;
+			if (res4 <= 0)
+				fmt = fmt3;
+
+			pr_warn("snprintf(%s%s); %d <= 0 (fmt %s)",
+				kind_name, name,
+				res1 <= 0 ? res1 : res2 <= 0 ? res2 :
+				res3 <= 0 ? res3 : res4, fmt);
+			failed_tests++;
+		} else if (res1 != res2) {
+			pr_warn("snprintf(%s%s): %d (to buf) != %d (no buf)",
+				kind_name, name, res1, res2);
+			failed_tests++;
+		} else if (res3 > res2) {
+			pr_warn("snprintf(%s%s); %d (no names) > %d (names)",
+				kind_name, name, res3, res2);
+			failed_tests++;
+		} else {
+			pr_debug("Printed %s%s (%d bytes)",
+				 kind_name, name, res1);
+		}
+	}
+	kfree(dummy_data);
+	kfree(buf);
+}
+
+/*
+ * For BTF it is the struct btf_ptr * ptr field, not the pointer itself
+ * which gets displayed, so it is that we need to hash.
+ */
+static void __init
+test_btf_hashed(const char *fmt, struct btf_ptr *p)
+{
+	char buf[PLAIN_BUF_SIZE];
+	int ret;
+
+	ret = plain_hash_to_buffer(p->ptr, buf, PLAIN_BUF_SIZE);
+	if (ret)
+		return;
+
+	test(buf, fmt, p);
+}
+
+#ifdef CONFIG_BTF_PRINTF
+
+static void __init btf_pointer_test_int(void)
+{
+	/* simple int */
+	TEST_BTF_C(int, testint, 1234);
+	TEST_BTF("cN", int, testint, "1234", 1234);
+	/* zero value should be printed at toplevel */
+	TEST_BTF("c", int, testint, "(int)0", 0);
+	TEST_BTF("cN", int, testint, "0", 0);
+	TEST_BTF("c0", int, testint, "(int)0", 0);
+	TEST_BTF("cN0", int, testint, "0", 0);
+	TEST_BTF_C(int, testint, -4567);
+	TEST_BTF("cN", int, testint, "-4567", -4567);
+}
+
+static void __init btf_pointer_test_char(void)
+{
+	/* simple char */
+	TEST_BTF_C(char, testchar, 100);
+	TEST_BTF("cN", char, testchar, "100", 100);
+	/* zero value should be printed at toplevel */
+	TEST_BTF("c", char, testchar, "(char)0", 0);
+	TEST_BTF("cN", char, testchar, "0", 0);
+	TEST_BTF("c0", char, testchar, "(char)0", 0);
+	TEST_BTF("cN0", char, testchar, "0", 0);
+}
+
+static void __init btf_pointer_test_typedef(void)
+{
+	/* simple typedef */
+	TEST_BTF_C(phys_addr_t, testtype, 100);
+	TEST_BTF("cN", phys_addr_t, testtype, "1", 1);
+	/* zero value should be printed at toplevel */
+	TEST_BTF("c", phys_addr_t, testtype, "(phys_addr_t)0", 0);
+	TEST_BTF("cN", phys_addr_t, testtype, "0", 0);
+	TEST_BTF("c0", phys_addr_t, testtype, "(phys_addr_t)0", 0);
+	TEST_BTF("cN0", phys_addr_t, testtype, "0", 0);
+
+	/* typedef struct */
+	TEST_BTF_C(atomic_t, testtype, {.counter = (int)1,});
+	TEST_BTF("cN", atomic_t, testtype, "{1,}", {.counter = 1,});
+	/* typedef with 0 value should be printed at toplevel */
+	TEST_BTF("c", atomic_t, testtype, "(atomic_t){}", {.counter = 0,});
+	TEST_BTF("cN", atomic_t, testtype, "{}", {.counter = 0,});
+	TEST_BTF("c0", atomic_t, testtype, "(atomic_t){.counter = (int)0,}",
+		 {.counter = 0,});
+	TEST_BTF("cN0", atomic_t, testtype, "{0,}", {.counter = 0,});
+
+	/* typedef array */
+	TEST_BTF("c", cpumask_t, testtype,
+		 "(cpumask_t){.bits = (long unsigned int[])[1,],}",
+		 { .bits = {1,}});
+	TEST_BTF("cN", cpumask_t, testtype, "{[1,],}",
+		 { .bits = {1,}});
+	/* typedef with 0 value should be printed at toplevel */
+	TEST_BTF("c", cpumask_t, testtype, "(cpumask_t){}", {{ 0 }});
+}
+
+static void __init btf_pointer_test_enum(void)
+{
+	/* enum where enum value does (and does not) exist */
+	TEST_BTF_C(enum bpf_cmd, testenum, BPF_MAP_CREATE);
+	TEST_BTF("c", enum bpf_cmd, testenum, "(enum bpf_cmd)BPF_MAP_CREATE",
+		 0);
+	TEST_BTF("cN", enum bpf_cmd, testenum, "BPF_MAP_CREATE",
+		 BPF_MAP_CREATE);
+	TEST_BTF("cN0", enum bpf_cmd, testenum, "BPF_MAP_CREATE", 0);
+
+	TEST_BTF("c0", enum bpf_cmd, testenum, "(enum bpf_cmd)BPF_MAP_CREATE",
+		 BPF_MAP_CREATE);
+	TEST_BTF("cN0", enum bpf_cmd, testenum, "BPF_MAP_CREATE",
+		 BPF_MAP_CREATE);
+	TEST_BTF_C(enum bpf_cmd, testenum, 2000);
+	TEST_BTF("cN", enum bpf_cmd, testenum, "2000", 2000);
+}
+
+static void __init btf_pointer_test_struct(void)
+{
+	/* simple struct */
+	TEST_BTF_C(struct btf_enum, teststruct,
+		   {.name_off = (__u32)3,.val = (__s32)-1,});
+	TEST_BTF("cN", struct btf_enum, teststruct, "{3,-1,}",
+		 { .name_off = 3, .val = -1,});
+	TEST_BTF("cN", struct btf_enum, teststruct, "{-1,}",
+		 { .name_off = 0, .val = -1,});
+	TEST_BTF("cN0", struct btf_enum, teststruct, "{0,-1,}",
+		 { .name_off = 0, .val = -1,});
+	/* empty struct should be printed */
+	TEST_BTF("c", struct btf_enum, teststruct, "(struct btf_enum){}",
+		 { .name_off = 0, .val = 0,});
+	TEST_BTF("cN", struct btf_enum, teststruct, "{}",
+		 { .name_off = 0, .val = 0,});
+	TEST_BTF("c0", struct btf_enum, teststruct,
+		 "(struct btf_enum){.name_off = (__u32)0,.val = (__s32)0,}",
+		 { .name_off = 0, .val = 0,});
+
+	/* struct with pointers */
+	TEST_BTF("cx", struct skb_shared_info, testptr,
+		 "(struct skb_shared_info){.frag_list = (struct sk_buff *)0000000000000001,}",
+		 { .frag_list = (struct sk_buff *)1 });
+	/* NULL pointer should not be displayed */
+	TEST_BTF("cx", struct skb_shared_info, testptr,
+		 "(struct skb_shared_info){}",
+		 { .frag_list = (struct sk_buff *)0 });
+
+	/* struct with char array */
+	TEST_BTF("c", struct bpf_prog_info, teststruct,
+		 "(struct bpf_prog_info){.name = (char[])['f','o','o',],}",
+		 { .name = "foo",});
+	TEST_BTF("cN", struct bpf_prog_info, teststruct,
+		 "{['f','o','o',],}",
+		 {.name = "foo",});
+	/* leading null char means do not display string */
+	TEST_BTF("c", struct bpf_prog_info, teststruct,
+		 "(struct bpf_prog_info){}",
+		 {.name = {'\0', 'f', 'o', 'o'}});
+	/* handle non-printable characters */
+	TEST_BTF("c", struct bpf_prog_info, teststruct,
+		 "(struct bpf_prog_info){.name = (char[])[1,2,3,],}",
+		 { .name = {1, 2, 3, 0}});
+
+	/* struct with non-char array */
+	TEST_BTF("c", struct __sk_buff, teststruct,
+		 "(struct __sk_buff){.cb = (__u32[])[1,2,3,4,5,],}",
+		 { .cb = {1, 2, 3, 4, 5,},});
+	TEST_BTF("cN", struct __sk_buff, teststruct,
+		 "{[1,2,3,4,5,],}",
+		 { .cb = { 1, 2, 3, 4, 5},});
+	/* For non-char, arrays, show non-zero values only */
+	TEST_BTF("c", struct __sk_buff, teststruct,
+		 "(struct __sk_buff){.cb = (__u32[])[1,],}",
+		 { .cb = { 0, 0, 1, 0, 0},});
+
+	/* struct with struct array */
+	TEST_BTF("c", struct bpf_struct_ops, teststruct,
+		 "(struct bpf_struct_ops){.func_models = (struct btf_func_model[])[(struct btf_func_model){.ret_size = (u8)1,.nr_args = (u8)2,.arg_size = (u8[])[3,4,5,],},],}",
+		 { .func_models = {{ .ret_size = 1, .nr_args = 2,
+				     .arg_size = { 3, 4, 5,},}}});
+
+	/* struct with bitfields */
+	TEST_BTF_C(struct bpf_insn, testbitfield,
+		   {.code = (__u8)1,.dst_reg = 0x2,.src_reg = 0x3,.off = (__s16)4,.imm = (__s32)5,});
+	TEST_BTF("cN", struct bpf_insn, testbitfield, "{1,0x2,0x3,4,5,}",
+		 {.code = 1, .dst_reg = 0x2, .src_reg = 0x3, .off = 4,
+		  .imm = 5,});
+
+	/* struct with anon struct/unions */
+	TEST_BTF("cx", struct sk_buff, test_anon,
+		 "(struct sk_buff){(union){(struct){(union){.dev = (struct net_device *)0000000000000001,.dev_scratch = (long unsigned int)1,},},.rbnode = (struct rb_node){.rb_left = (struct rb_node *)0000000000000001,},},}",
+		 { .dev_scratch = 1 });
+}
+
+#endif /* CONFIG_BTF_PRINTF */
+
+static void __init
+btf_pointer(void)
+{
+	struct sk_buff *skb = alloc_skb(64, GFP_KERNEL);
+	u8 fillvals[2] = { 0x00, 0xff };
+	int i;
+
+#ifdef CONFIG_BTF_PRINTF
+	btf_pointer_test_int();
+	btf_pointer_test_char();
+	btf_pointer_test_typedef();
+	btf_pointer_test_enum();
+	btf_pointer_test_struct();
+#endif /* CONFIG_BTF_PRINTF */
+
+	/*
+	 * Iterate every instance of each kind, printing each associated type.
+	 * This constitutes around 10k tests.
+	 */
+	for (i = 0; i < ARRAY_SIZE(fillvals); i++) {
+		btf_print_kind(BTF_KIND_STRUCT, "struct ", fillvals[i]);
+		btf_print_kind(BTF_KIND_UNION, "union ", fillvals[i]);
+		btf_print_kind(BTF_KIND_ENUM, "enum ", fillvals[i]);
+		btf_print_kind(BTF_KIND_TYPEDEF, "", fillvals[i]);
+	}
+
+	/* verify unknown type falls back to hashed pointer display */
+	test("(null)", "%pT", BTF_PTR_TYPE(NULL, "unknown_type"));
+	test_btf_hashed("%pT", BTF_PTR_TYPE(skb, "unknown_type"));
+
+	kfree_skb(skb);
+}
+
 static void __init
 test_pointer(void)
 {
@@ -668,6 +968,7 @@ static void __init fwnode_pointer(void)
 	flags();
 	errptr();
 	fwnode_pointer();
+	btf_pointer();
 }
 
 static void __init selftest(void)
-- 
1.8.3.1

[PATCH v2 bpf-next 6/7] bpf: add support for %pT format specifier for bpf_trace_printk() helper

[Alan Maguire]

Allow %pT[cNx0] format specifier for BTF-based display of data associated
with pointer.

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 include/uapi/linux/bpf.h       | 27 ++++++++++++++++++++++-----
 kernel/trace/bpf_trace.c       | 21 ++++++++++++++++++---
 tools/include/uapi/linux/bpf.h | 27 ++++++++++++++++++++++-----
 3 files changed, 62 insertions(+), 13 deletions(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 9d1932e..ab3c86c 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -695,7 +695,12 @@ struct bpf_stack_build_id {
  * 		to file *\/sys/kernel/debug/tracing/trace* from DebugFS, if
  * 		available. It can take up to three additional **u64**
  * 		arguments (as an eBPF helpers, the total number of arguments is
- * 		limited to five).
+ *		limited to five), and also supports %pT (BTF-based type
+ *		printing), as long as BPF_READ lockdown is not active.
+ *		"%pT" takes a "struct __btf_ptr *" as an argument; it
+ *		consists of a pointer value and specified BTF type string or id
+ *		used to select the type for display.  For more details, see
+ *		Documentation/core-api/printk-formats.rst.
  *
  * 		Each time the helper is called, it appends a line to the trace.
  * 		Lines are discarded while *\/sys/kernel/debug/tracing/trace* is
@@ -731,10 +736,10 @@ struct bpf_stack_build_id {
  * 		The conversion specifiers supported by *fmt* are similar, but
  * 		more limited than for printk(). They are **%d**, **%i**,
  * 		**%u**, **%x**, **%ld**, **%li**, **%lu**, **%lx**, **%lld**,
- * 		**%lli**, **%llu**, **%llx**, **%p**, **%s**. No modifier (size
- * 		of field, padding with zeroes, etc.) is available, and the
- * 		helper will return **-EINVAL** (but print nothing) if it
- * 		encounters an unknown specifier.
+ *		**%lli**, **%llu**, **%llx**, **%p**, **%pT[cNx0], **%s**.
+ *		Only %pT supports modifiers, and the helper will return
+ *		**-EINVAL** (but print nothing) if it encouters an unknown
+ *		specifier.
  *
  * 		Also, note that **bpf_trace_printk**\ () is slow, and should
  * 		only be used for debugging purposes. For this reason, a notice
@@ -4058,4 +4063,16 @@ struct bpf_pidns_info {
 	__u32 pid;
 	__u32 tgid;
 };
+
+/*
+ * struct __btf_ptr is used for %pT (typed pointer) display; the
+ * additional type string/BTF id are used to render the pointer
+ * data as the appropriate type.
+ */
+struct __btf_ptr {
+	void *ptr;
+	const char *type;
+	__u32 id;
+};
+
 #endif /* _UAPI__LINUX_BPF_H__ */
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index d961428..c032c58 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -321,9 +321,12 @@ static const struct bpf_func_proto *bpf_get_probe_write_proto(void)
 	return &bpf_probe_write_user_proto;
 }
 
+#define isbtffmt(c)	\
+	(c == 'T' || c == 'c' || c == 'N' || c == 'x' || c == '0')
+
 /*
  * Only limited trace_printk() conversion specifiers allowed:
- * %d %i %u %x %ld %li %lu %lx %lld %lli %llu %llx %p %s
+ * %d %i %u %x %ld %li %lu %lx %lld %lli %llu %llx %p %pT %s
  */
 BPF_CALL_5(bpf_trace_printk, char *, fmt, u32, fmt_size, u64, arg1,
 	   u64, arg2, u64, arg3)
@@ -361,8 +364,20 @@ static const struct bpf_func_proto *bpf_get_probe_write_proto(void)
 			i++;
 		} else if (fmt[i] == 'p' || fmt[i] == 's') {
 			mod[fmt_cnt]++;
-			/* disallow any further format extensions */
-			if (fmt[i + 1] != 0 &&
+			/*
+			 * allow BTF type-based printing, and disallow any
+			 * further format extensions.
+			 */
+			if (fmt[i] == 'p' && fmt[i + 1] == 'T') {
+				int ret;
+
+				ret = security_locked_down(LOCKDOWN_BPF_READ);
+				if (unlikely(ret < 0))
+					return ret;
+				i++;
+				while (isbtffmt(fmt[i]))
+					i++;
+			} else if (fmt[i + 1] != 0 &&
 			    !isspace(fmt[i + 1]) &&
 			    !ispunct(fmt[i + 1]))
 				return -EINVAL;
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 9d1932e..ab3c86c 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -695,7 +695,12 @@ struct bpf_stack_build_id {
  * 		to file *\/sys/kernel/debug/tracing/trace* from DebugFS, if
  * 		available. It can take up to three additional **u64**
  * 		arguments (as an eBPF helpers, the total number of arguments is
- * 		limited to five).
+ *		limited to five), and also supports %pT (BTF-based type
+ *		printing), as long as BPF_READ lockdown is not active.
+ *		"%pT" takes a "struct __btf_ptr *" as an argument; it
+ *		consists of a pointer value and specified BTF type string or id
+ *		used to select the type for display.  For more details, see
+ *		Documentation/core-api/printk-formats.rst.
  *
  * 		Each time the helper is called, it appends a line to the trace.
  * 		Lines are discarded while *\/sys/kernel/debug/tracing/trace* is
@@ -731,10 +736,10 @@ struct bpf_stack_build_id {
  * 		The conversion specifiers supported by *fmt* are similar, but
  * 		more limited than for printk(). They are **%d**, **%i**,
  * 		**%u**, **%x**, **%ld**, **%li**, **%lu**, **%lx**, **%lld**,
- * 		**%lli**, **%llu**, **%llx**, **%p**, **%s**. No modifier (size
- * 		of field, padding with zeroes, etc.) is available, and the
- * 		helper will return **-EINVAL** (but print nothing) if it
- * 		encounters an unknown specifier.
+ *		**%lli**, **%llu**, **%llx**, **%p**, **%pT[cNx0], **%s**.
+ *		Only %pT supports modifiers, and the helper will return
+ *		**-EINVAL** (but print nothing) if it encouters an unknown
+ *		specifier.
  *
  * 		Also, note that **bpf_trace_printk**\ () is slow, and should
  * 		only be used for debugging purposes. For this reason, a notice
@@ -4058,4 +4063,16 @@ struct bpf_pidns_info {
 	__u32 pid;
 	__u32 tgid;
 };
+
+/*
+ * struct __btf_ptr is used for %pT (typed pointer) display; the
+ * additional type string/BTF id are used to render the pointer
+ * data as the appropriate type.
+ */
+struct __btf_ptr {
+	void *ptr;
+	const char *type;
+	__u32 id;
+};
+
 #endif /* _UAPI__LINUX_BPF_H__ */
-- 
1.8.3.1

[PATCH v2 bpf-next 7/7] bpf: add tests for %pT format specifier

[Alan Maguire]

tests verify we get > 0 return value from bpf_trace_print()
using %pT format specifier with various modifiers/pointer
values.

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 .../selftests/bpf/prog_tests/trace_printk_btf.c    | 83 ++++++++++++++++++++++
 .../selftests/bpf/progs/netif_receive_skb.c        | 81 +++++++++++++++++++++
 2 files changed, 164 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/trace_printk_btf.c
 create mode 100644 tools/testing/selftests/bpf/progs/netif_receive_skb.c

diff --git a/tools/testing/selftests/bpf/prog_tests/trace_printk_btf.c b/tools/testing/selftests/bpf/prog_tests/trace_printk_btf.c
new file mode 100644
index 0000000..d7ee158
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/trace_printk_btf.c
@@ -0,0 +1,83 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <test_progs.h>
+
+struct result {
+	int ret;
+	int subtest;
+	int num_subtest;
+};
+
+/* return value of bpf_trace_printk()s is stored; if nonzero we failed. */
+static void on_sample(void *ctx, int cpu, void *data, __u32 size)
+{
+	struct result *resp = (struct result *)data;
+
+	*(struct result *)ctx = *resp;
+}
+
+void test_trace_printk_btf(void)
+{
+	struct result res = { 0 };
+	struct bpf_prog_load_attr attr = {
+		.file = "./netif_receive_skb.o",
+	};
+	struct perf_buffer_opts pb_opts = {};
+	struct bpf_program *prog = NULL;
+	struct perf_buffer *pb = NULL;
+	struct bpf_object *obj = NULL;
+	struct bpf_link *link = NULL;
+	struct bpf_map *perf_buf_map;
+	__u32 duration = 0;
+	int err, prog_fd;
+
+	err = bpf_prog_load_xattr(&attr, &obj, &prog_fd);
+	if (CHECK(err, "prog_load raw tp", "err %d errno %d\n", err, errno))
+		goto close_prog;
+
+	prog = bpf_object__find_program_by_title(obj,
+						 "tp_btf/netif_receive_skb");
+	if (CHECK(!prog, "find_prog", "prog netif_receive_skb not found\n"))
+		goto close_prog;
+
+	link = bpf_program__attach_raw_tracepoint(prog, NULL);
+	if (CHECK(IS_ERR(link), "attach_raw_tp", "err %ld\n", PTR_ERR(link)))
+		goto close_prog;
+
+	perf_buf_map = bpf_object__find_map_by_name(obj, "perf_buf_map");
+	if (CHECK(!perf_buf_map, "find_perf_buf_map", "not found\n"))
+		goto close_prog;
+
+	/* set up perf buffer */
+	pb_opts.sample_cb = on_sample;
+	pb_opts.ctx = &res;
+	pb = perf_buffer__new(bpf_map__fd(perf_buf_map), 1, &pb_opts);
+	if (CHECK(IS_ERR(pb), "perf_buf__new", "err %ld\n", PTR_ERR(pb)))
+		goto close_prog;
+
+	/* generate receive event */
+	system("ping -c 1 127.0.0.1 >/dev/null");
+
+	/* read perf buffer */
+	err = perf_buffer__poll(pb, 100);
+	if (CHECK(err < 0, "perf_buffer__poll", "err %d\n", err))
+		goto close_prog;
+
+	/*
+	 * Make sure netif_receive_skb program was triggered
+	 * and it sent expected return values from bpf_trace_printk()s
+	 * into ring buffer.
+	 */
+	if (CHECK(res.ret <= 0,
+		  "bpf_trace_printk: got return value",
+		  "ret <= 0 %d test %d\n", res.ret, res.subtest))
+		goto close_prog;
+
+	CHECK(res.subtest != res.num_subtest, "check all subtests ran",
+	      "only ran %d of %d tests\n", res.subtest, res.num_subtest);
+
+close_prog:
+	perf_buffer__free(pb);
+	if (!IS_ERR_OR_NULL(link))
+		bpf_link__destroy(link);
+	bpf_object__close(obj);
+}
diff --git a/tools/testing/selftests/bpf/progs/netif_receive_skb.c b/tools/testing/selftests/bpf/progs/netif_receive_skb.c
new file mode 100644
index 0000000..b5148df
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/netif_receive_skb.c
@@ -0,0 +1,81 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2020, Oracle and/or its affiliates. */
+#include <linux/bpf.h>
+#include <stdbool.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_endian.h>
+#include <bpf/bpf_tracing.h>
+
+char _license[] SEC("license") = "GPL";
+
+struct {
+	__uint(type, BPF_MAP_TYPE_PERF_EVENT_ARRAY);
+	__uint(key_size, sizeof(int));
+	__uint(value_size, sizeof(int));
+} perf_buf_map SEC(".maps");
+
+struct result {
+	int ret;
+	int subtest;
+	int num_subtest;
+};
+
+typedef struct {
+	int counter;
+} atomic_t;
+typedef struct refcount_struct {
+	atomic_t refs;
+} refcount_t;
+
+struct sk_buff {
+	/* field names and sizes should match to those in the kernel */
+	unsigned int len, data_len;
+	__u16 mac_len, hdr_len, queue_mapping;
+	struct net_device *dev;
+	/* order of the fields doesn't matter */
+	refcount_t users;
+	unsigned char *data;
+	char __pkt_type_offset[0];
+	char cb[48];
+};
+
+#define CHECK_PRINTK(_fmt, _p, res)					\
+	do {								\
+		char fmt[] = _fmt;					\
+		++(res)->num_subtest;					\
+		if ((res)->ret >= 0) {					\
+			++(res)->subtest;				\
+			(res)->ret = bpf_trace_printk(fmt, sizeof(fmt),	\
+						      (_p));		\
+		}							\
+	} while (0)
+
+/* TRACE_EVENT(netif_receive_skb,
+ *	TP_PROTO(struct sk_buff *skb),
+ */
+SEC("tp_btf/netif_receive_skb")
+int BPF_PROG(trace_netif_receive_skb, struct sk_buff *skb)
+{
+	char skb_type[] = "struct sk_buff";
+	struct __btf_ptr nullp = { .ptr = 0, .type = skb_type };
+	struct __btf_ptr p = { .ptr = skb, .type = skb_type };
+	struct result res = { 0, 0 };
+
+	CHECK_PRINTK("%pT\n", &p, &res);
+	CHECK_PRINTK("%pTc\n", &p, &res);
+	CHECK_PRINTK("%pTN\n", &p, &res);
+	CHECK_PRINTK("%pTx\n", &p, &res);
+	CHECK_PRINTK("%pT0\n", &p, &res);
+	CHECK_PRINTK("%pTcNx0\n", &p, &res);
+	CHECK_PRINTK("%pT\n", &nullp, &res);
+	CHECK_PRINTK("%pTc\n", &nullp, &res);
+	CHECK_PRINTK("%pTN\n", &nullp, &res);
+	CHECK_PRINTK("%pTx\n", &nullp, &res);
+	CHECK_PRINTK("%pT0\n", &nullp, &res);
+	CHECK_PRINTK("%pTcNx0\n", &nullp, &res);
+
+	bpf_perf_event_output(ctx, &perf_buf_map, BPF_F_CURRENT_CPU,
+			      &res, sizeof(res));
+
+	return 0;
+}
-- 
1.8.3.1

Re: [PATCH v2 bpf-next 0/7] bpf, printk: add BTF-based type printing

[Alexei Starovoitov]

On Tue, May 12, 2020 at 06:56:38AM +0100, Alan Maguire wrote:
> The printk family of functions support printing specific pointer types
> using %p format specifiers (MAC addresses, IP addresses, etc).  For
> full details see Documentation/core-api/printk-formats.rst.
> 
> This patchset proposes introducing a "print typed pointer" format
> specifier "%pT"; the argument associated with the specifier is of
> form "struct btf_ptr *" which consists of a .ptr value and a .type
> value specifying a stringified type (e.g. "struct sk_buff") or
> an .id value specifying a BPF Type Format (BTF) id identifying
> the appropriate type it points to.
> 
> There is already support in kernel/bpf/btf.c for "show" functionality;
> the changes here generalize that support from seq-file specific
> verifier display to the more generic case and add another specific
> use case; snprintf()-style rendering of type information to a
> provided buffer.  This support is then used to support printk
> rendering of types, but the intent is to provide a function
> that might be useful in other in-kernel scenarios; for example:
> 
> - ftrace could possibly utilize the function to support typed
>   display of function arguments by cross-referencing BTF function
>   information to derive the types of arguments
> - oops/panic messaging could extend the information displayed to
>   dig into data structures associated with failing functions
> 
> The above potential use cases hint at a potential reply to
> a reasonable objection that such typed display should be
> solved by tracing programs, where the in kernel tracing records
> data and the userspace program prints it out.  While this
> is certainly the recommended approach for most cases, I
> believe having an in-kernel mechanism would be valuable
> also.
> 
> The function the printk() family of functions rely on
> could potentially be used directly for other use cases
> like ftrace where we might have the BTF ids of the
> pointers we wish to display; its signature is as follows:
> 
> int btf_type_snprintf_show(const struct btf *btf, u32 type_id, void *obj,
>                            char *buf, int len, u64 flags);
> 
> So if ftrace say had the BTF ids of the types of arguments,
> we see that the above would allow us to convert the
> pointer data into displayable form.
> 
> To give a flavour for what the printed-out data looks like,
> here we use pr_info() to display a struct sk_buff *.
> 
>   struct sk_buff *skb = alloc_skb(64, GFP_KERNEL);
> 
>   pr_info("%pT", BTF_PTR_TYPE(skb, "struct sk_buff"));
> 
> ...gives us:
> 
> (struct sk_buff){
>  .transport_header = (__u16)65535,
>  .mac_header = (__u16)65535,
>  .end = (sk_buff_data_t)192,
>  .head = (unsigned char *)000000007524fd8b,
>  .data = (unsigned char *)000000007524fd8b,

could you add "0x" prefix here to make it even more C like
and unambiguous ?

>  .truesize = (unsigned int)768,
>  .users = (refcount_t){
>   .refs = (atomic_t){
>    .counter = (int)1,
>   },
>  },
> }
> 
> For bpf_trace_printk() a "struct __btf_ptr *" is used as
> argument; see tools/testing/selftests/bpf/progs/netif_receive_skb.c
> for example usage.
> 
> The hope is that this functionality will be useful for debugging,
> and possibly help facilitate the cases mentioned above in the future.
> 
> Changes since v1:
> 
> - changed format to be more drgn-like, rendering indented type info
>   along with type names by default (Alexei)
> - zeroed values are omitted (Arnaldo) by default unless the '0'
>   modifier is specified (Alexei)
> - added an option to print pointer values without obfuscation.
>   The reason to do this is the sysctls controlling pointer display
>   are likely to be irrelevant in many if not most tracing contexts.
>   Some questions on this in the outstanding questions section below...
> - reworked printk format specifer so that we no longer rely on format
>   %pT<type> but instead use a struct * which contains type information
>   (Rasmus). This simplifies the printk parsing, makes use more dynamic
>   and also allows specification by BTF id as well as name.
> - ensured that BTF-specific printk code is bracketed by
>   #if ENABLED(CONFIG_BTF_PRINTF)

I don't like this particular bit:
+config BTF_PRINTF
+       bool "print type information using BPF type format"
+       depends on DEBUG_INFO_BTF
+       default n

Looks like it's only purpose is to gate printk test.
In such case please convert it into hidden config that is
automatically selected when DEBUG_INFO_BTF is set.
Or just make printk tests to #if IS_ENABLED(DEBUG_INFO_BTF)

> - removed incorrect patch which tried to fix dereferencing of resolved
>   BTF info for vmlinux; instead we skip modifiers for the relevant
>   case (array element type/size determination) (Alexei).
> - fixed issues with negative snprintf format length (Rasmus)
> - added test cases for various data structure formats; base types,
>   typedefs, structs, etc.
> - tests now iterate through all typedef, enum, struct and unions
>   defined for vmlinux BTF and render a version of the target dummy
>   value which is either all zeros or all 0xff values; the idea is this
>   exercises the "skip if zero" and "print everything" cases.
> - added support in BPF for using the %pT format specifier in
>   bpf_trace_printk()
> - added BPF tests which ensure %pT format specifier use works (Alexei).
> 
> Outstanding issues
> 
> - currently %pT is not allowed in BPF programs when lockdown is active
>   prohibiting BPF_READ; is that sufficient?

yes. that's enough.

> - do we want to further restrict the non-obfuscated pointer format
>   specifier %pTx; for example blocking unprivileged BPF programs from
>   using it?

unpriv cannot use bpf_trace_printk() anyway. I'm not sure what is the concern.

> - likely still need a better answer for vmlinux BTF initialization
>   than the current approach taken; early boot initialization is one
>   way to go here.

btf_parse_vmlinux() can certainly be moved to late_initcall().

> - may be useful to have a "print integers as hex" format modifier (Joe)

I'd rather stick to the convention that the type drives the way the value is printed.
For example:
  u8 ch = 65;
  pr_info("%pT", BTF_PTR_TYPE(&ch, "char"));
  prints 'A'
while
  u8 ch = 65;
  pr_info("%pT", BTF_PTR_TYPE(&ch, "u8"));
  prints 65

> 
> Important note: if running test_printf.ko - the version in the bpf-next
> tree will induce a panic when running the fwnode_pointer() tests due
> to a kobject issue; applying the patch in
> 
> https://lkml.org/lkml/2020/4/17/389

Thanks for the headsup!

BTF_PTR_ID() is a great idea as well.
In the llvm 11 we will introduce __builtin__btf_type_id().
The bpf program will be able to say:
bpf_printk("%pT", BTF_PTR_ID(skb, __builtin_btf_type_id(skb, 1)));
That will help avoid run-time search through btf types by string name.
The compiler will supply btf_id at build time and libbpf will do
a relocation into vmlinux btf_id prior to loading.

Also I think there should be another flag BTF_SHOW_PROBE_DATA.
It should probe_kernel_read() all data instead of direct dereferences.
It could be implemented via single probe_kernel_read of the whole BTF data
structure before pringing the fields one by one. So it should be simple
addition to patch 2.
This flag should be default for printk() from bpf program,
since the verifier won't be checking that pointer passed into bpf_trace_printk
is of correct btf type and it's a real pointer.
Whether this flag should be default for normal printk() is arguable.
I would make it so. The performance cost of extra copy is small comparing
with no-crash guarantee it provides. I think it would be a good trade off.

In the future we can extend the verifier so that:
bpf_safe_printk("%pT", skb);
will be recognized that 'skb' is PTR_TO_BTF_ID and corresponding and correct
btf_id is passed into printk. Mistakes will be caught at program
verification time.

Re: [PATCH v2 bpf-next 0/7] bpf, printk: add BTF-based type printing

[Joe Perches]

On Wed, 2020-05-13 at 15:24 -0700, Alexei Starovoitov wrote:
> On Tue, May 12, 2020 at 06:56:38AM +0100, Alan Maguire wrote:
> > The printk family of functions support printing specific pointer types
> > using %p format specifiers (MAC addresses, IP addresses, etc).  For
> > full details see Documentation/core-api/printk-formats.rst.
> > 
> > This patchset proposes introducing a "print typed pointer" format
> > specifier "%pT"; the argument associated with the specifier is of
> > form "struct btf_ptr *" which consists of a .ptr value and a .type
> > value specifying a stringified type (e.g. "struct sk_buff") or
> > an .id value specifying a BPF Type Format (BTF) id identifying
> > the appropriate type it points to.
> > 
> >   pr_info("%pT", BTF_PTR_TYPE(skb, "struct sk_buff"));
> > 
> > ...gives us:
> > 
> > (struct sk_buff){
> >  .transport_header = (__u16)65535,
> >  .mac_header = (__u16)65535,
> >  .end = (sk_buff_data_t)192,
> >  .head = (unsigned char *)000000007524fd8b,
> >  .data = (unsigned char *)000000007524fd8b,
> 
> could you add "0x" prefix here to make it even more C like
> and unambiguous ?

linux pointers are not emitted with an 0x prefix

(ie: pointers do not use SPECIAL in lib/vsprintf.c)

Re: [PATCH v2 bpf-next 0/7] bpf, printk: add BTF-based type printing

[Alexei Starovoitov]

On Wed, May 13, 2020 at 3:48 PM Joe Perches <joe@perches.com> wrote:
>
> On Wed, 2020-05-13 at 15:24 -0700, Alexei Starovoitov wrote:
> > On Tue, May 12, 2020 at 06:56:38AM +0100, Alan Maguire wrote:
> > > The printk family of functions support printing specific pointer types
> > > using %p format specifiers (MAC addresses, IP addresses, etc).  For
> > > full details see Documentation/core-api/printk-formats.rst.
> > >
> > > This patchset proposes introducing a "print typed pointer" format
> > > specifier "%pT"; the argument associated with the specifier is of
> > > form "struct btf_ptr *" which consists of a .ptr value and a .type
> > > value specifying a stringified type (e.g. "struct sk_buff") or
> > > an .id value specifying a BPF Type Format (BTF) id identifying
> > > the appropriate type it points to.
> > >
> > >   pr_info("%pT", BTF_PTR_TYPE(skb, "struct sk_buff"));
> > >
> > > ...gives us:
> > >
> > > (struct sk_buff){
> > >  .transport_header = (__u16)65535,
> > >  .mac_header = (__u16)65535,
> > >  .end = (sk_buff_data_t)192,
> > >  .head = (unsigned char *)000000007524fd8b,
> > >  .data = (unsigned char *)000000007524fd8b,
> >
> > could you add "0x" prefix here to make it even more C like
> > and unambiguous ?
>
> linux pointers are not emitted with an 0x prefix

So? This is not at all comparable to %p

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

[Yonghong Song]

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);
> +}
> +
[...]

Re: [PATCH v2 bpf-next 4/7] printk: add type-printing %pT format specifier which uses BTF

[Joe Perches]

On Tue, 2020-05-12 at 06:56 +0100, Alan Maguire wrote:
> printk supports multiple pointer object type specifiers (printing
> netdev features etc).  Extend this support using BTF to cover
> arbitrary types.  "%pT" specifies the typed format, and the pointer
> argument is a "struct btf_ptr *" where struct btf_ptr is as follows:
> 
> struct btf_ptr {
> 	void *ptr;
> 	const char *type;
> 	u32 id;
> };
> 
> Either the "type" string ("struct sk_buff") or the BTF "id" can be
> used to identify the type to use in displaying the associated "ptr"
> value.  A convenience function to create and point at the struct
> is provided:
> 
> 	printk(KERN_INFO "%pT", BTF_PTR_TYPE(skb, struct sk_buff));
> 
> When invoked, BTF information is used to traverse the sk_buff *
> and display it.  Support is present for structs, unions, enums,
> typedefs and core types (though in the latter case there's not
> much value in using this feature of course).
> 
> Default output is indented, but compact output can be specified
> via the 'c' option.  Type names/member values can be suppressed
> using the 'N' option.  Zero values are not displayed by default
> but can be using the '0' option.  Pointer values are obfuscated
> unless the 'x' option is specified.  As an example:
> 
>   struct sk_buff *skb = alloc_skb(64, GFP_KERNEL);
>   pr_info("%pT", BTF_PTR_TYPE(skb, struct sk_buff));
> 
> ...gives us:
> 
> (struct sk_buff){
>  .transport_header = (__u16)65535,
> 	 .mac_header = (__u16)65535,
>  .end = (sk_buff_data_t)192,
>  .head = (unsigned char *)000000006b71155a,
>  .data = (unsigned char *)000000006b71155a,
>  .truesize = (unsigned int)768,
>  .users = (refcount_t){
>   .refs = (atomic_t){
>    .counter = (int)1,

Given

  #define BTF_INT_ENCODING(VAL)   (((VAL) & 0x0f000000) >> 24)

Maybe

  #define BTF_INT_SIGNED  (1 << 0)
  #define BTF_INT_CHAR    (1 << 1)
  #define BTF_INT_BOOL    (1 << 2)

could be extended to include

  #define BTF_INT_HEX     (1 << 3)

So hex values can be appropriately pretty-printed.

Re: [PATCH v2 bpf-next 4/7] printk: add type-printing %pT format specifier which uses BTF

[Alexei Starovoitov]

On Wed, May 13, 2020 at 4:05 PM Joe Perches <joe@perches.com> wrote:
>
> On Tue, 2020-05-12 at 06:56 +0100, Alan Maguire wrote:
> > printk supports multiple pointer object type specifiers (printing
> > netdev features etc).  Extend this support using BTF to cover
> > arbitrary types.  "%pT" specifies the typed format, and the pointer
> > argument is a "struct btf_ptr *" where struct btf_ptr is as follows:
> >
> > struct btf_ptr {
> >       void *ptr;
> >       const char *type;
> >       u32 id;
> > };
> >
> > Either the "type" string ("struct sk_buff") or the BTF "id" can be
> > used to identify the type to use in displaying the associated "ptr"
> > value.  A convenience function to create and point at the struct
> > is provided:
> >
> >       printk(KERN_INFO "%pT", BTF_PTR_TYPE(skb, struct sk_buff));
> >
> > When invoked, BTF information is used to traverse the sk_buff *
> > and display it.  Support is present for structs, unions, enums,
> > typedefs and core types (though in the latter case there's not
> > much value in using this feature of course).
> >
> > Default output is indented, but compact output can be specified
> > via the 'c' option.  Type names/member values can be suppressed
> > using the 'N' option.  Zero values are not displayed by default
> > but can be using the '0' option.  Pointer values are obfuscated
> > unless the 'x' option is specified.  As an example:
> >
> >   struct sk_buff *skb = alloc_skb(64, GFP_KERNEL);
> >   pr_info("%pT", BTF_PTR_TYPE(skb, struct sk_buff));
> >
> > ...gives us:
> >
> > (struct sk_buff){
> >  .transport_header = (__u16)65535,
> >        .mac_header = (__u16)65535,
> >  .end = (sk_buff_data_t)192,
> >  .head = (unsigned char *)000000006b71155a,
> >  .data = (unsigned char *)000000006b71155a,
> >  .truesize = (unsigned int)768,
> >  .users = (refcount_t){
> >   .refs = (atomic_t){
> >    .counter = (int)1,
>
> Given
>
>   #define BTF_INT_ENCODING(VAL)   (((VAL) & 0x0f000000) >> 24)
>
> Maybe
>
>   #define BTF_INT_SIGNED  (1 << 0)
>   #define BTF_INT_CHAR    (1 << 1)
>   #define BTF_INT_BOOL    (1 << 2)
>
> could be extended to include
>
>   #define BTF_INT_HEX     (1 << 3)
>
> So hex values can be appropriately pretty-printed.

Nack to that.

Re: [PATCH v2 bpf-next 4/7] printk: add type-printing %pT format specifier which uses BTF

[Joe Perches]

On Wed, 2020-05-13 at 16:07 -0700, Alexei Starovoitov wrote:
> On Wed, May 13, 2020 at 4:05 PM Joe Perches <joe@perches.com> wrote:
> > On Tue, 2020-05-12 at 06:56 +0100, Alan Maguire wrote:
> > > printk supports multiple pointer object type specifiers (printing
> > > netdev features etc).  Extend this support using BTF to cover
> > > arbitrary types.  "%pT" specifies the typed format, and the pointer
> > > argument is a "struct btf_ptr *" where struct btf_ptr is as follows:
> > > 
> > > struct btf_ptr {
> > >       void *ptr;
> > >       const char *type;
> > >       u32 id;
> > > };
> > > 
> > > Either the "type" string ("struct sk_buff") or the BTF "id" can be
> > > used to identify the type to use in displaying the associated "ptr"
> > > value.  A convenience function to create and point at the struct
> > > is provided:
> > > 
> > >       printk(KERN_INFO "%pT", BTF_PTR_TYPE(skb, struct sk_buff));
> > > 
> > > When invoked, BTF information is used to traverse the sk_buff *
> > > and display it.  Support is present for structs, unions, enums,
> > > typedefs and core types (though in the latter case there's not
> > > much value in using this feature of course).
> > > 
> > > Default output is indented, but compact output can be specified
> > > via the 'c' option.  Type names/member values can be suppressed
> > > using the 'N' option.  Zero values are not displayed by default
> > > but can be using the '0' option.  Pointer values are obfuscated
> > > unless the 'x' option is specified.  As an example:
> > > 
> > >   struct sk_buff *skb = alloc_skb(64, GFP_KERNEL);
> > >   pr_info("%pT", BTF_PTR_TYPE(skb, struct sk_buff));
> > > 
> > > ...gives us:
> > > 
> > > (struct sk_buff){
> > >  .transport_header = (__u16)65535,
> > >        .mac_header = (__u16)65535,
> > >  .end = (sk_buff_data_t)192,
> > >  .head = (unsigned char *)000000006b71155a,
> > >  .data = (unsigned char *)000000006b71155a,
> > >  .truesize = (unsigned int)768,
> > >  .users = (refcount_t){
> > >   .refs = (atomic_t){
> > >    .counter = (int)1,
> > 
> > Given
> > 
> >   #define BTF_INT_ENCODING(VAL)   (((VAL) & 0x0f000000) >> 24)
> > 
> > Maybe
> > 
> >   #define BTF_INT_SIGNED  (1 << 0)
> >   #define BTF_INT_CHAR    (1 << 1)
> >   #define BTF_INT_BOOL    (1 << 2)
> > 
> > could be extended to include
> > 
> >   #define BTF_INT_HEX     (1 << 3)
> > 
> > So hex values can be appropriately pretty-printed.
> 
> Nack to that.

why?

Re: [PATCH v2 bpf-next 0/7] bpf, printk: add BTF-based type printing

[Joe Perches]

On Wed, 2020-05-13 at 15:50 -0700, Alexei Starovoitov wrote:
> On Wed, May 13, 2020 at 3:48 PM Joe Perches <joe@perches.com> wrote:
> > On Wed, 2020-05-13 at 15:24 -0700, Alexei Starovoitov wrote:
> > > On Tue, May 12, 2020 at 06:56:38AM +0100, Alan Maguire wrote:
> > > > The printk family of functions support printing specific pointer types
> > > > using %p format specifiers (MAC addresses, IP addresses, etc).  For
> > > > full details see Documentation/core-api/printk-formats.rst.
> > > > 
> > > > This patchset proposes introducing a "print typed pointer" format
> > > > specifier "%pT"; the argument associated with the specifier is of
> > > > form "struct btf_ptr *" which consists of a .ptr value and a .type
> > > > value specifying a stringified type (e.g. "struct sk_buff") or
> > > > an .id value specifying a BPF Type Format (BTF) id identifying
> > > > the appropriate type it points to.
> > > > 
> > > >   pr_info("%pT", BTF_PTR_TYPE(skb, "struct sk_buff"));
> > > > 
> > > > ...gives us:
> > > > 
> > > > (struct sk_buff){
> > > >  .transport_header = (__u16)65535,
> > > >  .mac_header = (__u16)65535,
> > > >  .end = (sk_buff_data_t)192,
> > > >  .head = (unsigned char *)000000007524fd8b,
> > > >  .data = (unsigned char *)000000007524fd8b,
> > > 
> > > could you add "0x" prefix here to make it even more C like
> > > and unambiguous ?
> > 
> > linux pointers are not emitted with an 0x prefix
> 
> So? This is not at all comparable to %p

Then why is x used to obfuscate?

Re: [PATCH v2 bpf-next 4/7] printk: add type-printing %pT format specifier which uses BTF

[Yonghong Song]

On 5/11/20 10:56 PM, Alan Maguire wrote:
> printk supports multiple pointer object type specifiers (printing
> netdev features etc).  Extend this support using BTF to cover
> arbitrary types.  "%pT" specifies the typed format, and the pointer
> argument is a "struct btf_ptr *" where struct btf_ptr is as follows:
> 
> struct btf_ptr {
> 	void *ptr;
> 	const char *type;
> 	u32 id;
> };
> 
> Either the "type" string ("struct sk_buff") or the BTF "id" can be
> used to identify the type to use in displaying the associated "ptr"
> value.  A convenience function to create and point at the struct
> is provided:
> 
> 	printk(KERN_INFO "%pT", BTF_PTR_TYPE(skb, struct sk_buff));
> 
> When invoked, BTF information is used to traverse the sk_buff *
> and display it.  Support is present for structs, unions, enums,
> typedefs and core types (though in the latter case there's not
> much value in using this feature of course).
> 
> Default output is indented, but compact output can be specified
> via the 'c' option.  Type names/member values can be suppressed
> using the 'N' option.  Zero values are not displayed by default
> but can be using the '0' option.  Pointer values are obfuscated
> unless the 'x' option is specified.  As an example:
> 
>    struct sk_buff *skb = alloc_skb(64, GFP_KERNEL);
>    pr_info("%pT", BTF_PTR_TYPE(skb, struct sk_buff));
> 
> ...gives us:
> 
> (struct sk_buff){
>   .transport_header = (__u16)65535,
>   .mac_header = (__u16)65535,
>   .end = (sk_buff_data_t)192,
>   .head = (unsigned char *)000000006b71155a,
>   .data = (unsigned char *)000000006b71155a,
>   .truesize = (unsigned int)768,
>   .users = (refcount_t){
>    .refs = (atomic_t){
>     .counter = (int)1,
>    },
>   },
>   .extensions = (struct skb_ext *)00000000f486a130,
> }
> 
> printk output is truncated at 1024 bytes.  For cases where overflow
> is likely, the compact/no type names display modes may be used.
> 
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---
>   Documentation/core-api/printk-formats.rst |  15 ++++
>   include/linux/btf.h                       |   3 +-
>   include/linux/printk.h                    |  16 +++++
>   lib/Kconfig                               |  16 +++++
>   lib/vsprintf.c                            | 113 ++++++++++++++++++++++++++++++
>   5 files changed, 162 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
> index 8ebe46b1..5c66097 100644
> --- a/Documentation/core-api/printk-formats.rst
> +++ b/Documentation/core-api/printk-formats.rst
> @@ -545,6 +545,21 @@ For printing netdev_features_t.
>   
>   Passed by reference.
>   
> +BTF-based printing of pointer data
> +----------------------------------
> +If '%pT' is specified, use the struct btf_ptr * along with kernel vmlinux
> +BPF Type Format (BTF) to show the typed data.  For example, specifying
> +
> +	printk(KERN_INFO "%pT", BTF_PTR_TYPE(skb, struct_sk_buff));
> +
> +will utilize BTF information to traverse the struct sk_buff * and display it.
> +
> +Supported modifers are
> + 'c' compact output (no indentation, newlines etc)
> + 'N' do not show type names
> + 'x' show raw pointers (no obfuscation)
> + '0' show zero-valued data (it is not shown by default)
> +
>   Thanks
>   ======
>   
> diff --git a/include/linux/btf.h b/include/linux/btf.h
> index d571125..7b585ab 100644
> --- a/include/linux/btf.h
> +++ b/include/linux/btf.h
> @@ -169,10 +169,11 @@ static inline const struct btf_member *btf_type_member(const struct btf_type *t)
>   	return (const struct btf_member *)(t + 1);
>   }
>   
> +struct btf *btf_parse_vmlinux(void);
> +
>   #ifdef CONFIG_BPF_SYSCALL
>   const struct btf_type *btf_type_by_id(const struct btf *btf, u32 type_id);
>   const char *btf_name_by_offset(const struct btf *btf, u32 offset);
> -struct btf *btf_parse_vmlinux(void);
>   struct btf *bpf_prog_get_target_btf(const struct bpf_prog *prog);
>   #else
>   static inline const struct btf_type *btf_type_by_id(const struct btf *btf,
> diff --git a/include/linux/printk.h b/include/linux/printk.h
> index fcde0772..3c3ea53 100644
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -528,4 +528,20 @@ static inline void print_hex_dump_debug(const char *prefix_str, int prefix_type,
>   #define print_hex_dump_bytes(prefix_str, prefix_type, buf, len)	\
>   	print_hex_dump_debug(prefix_str, prefix_type, 16, 1, buf, len, true)
>   
> +/**
> + * struct btf_ptr is used for %pT (typed pointer) display; the
> + * additional type string/BTF id are used to render the pointer
> + * data as the appropriate type.
> + */
> +struct btf_ptr {
> +	void *ptr;
> +	const char *type;
> +	u32 id;
> +};
> +
> +#define	BTF_PTR_TYPE(ptrval, typeval) \
> +	(&((struct btf_ptr){.ptr = ptrval, .type = #typeval}))
> +
> +#define BTF_PTR_ID(ptrval, idval) \
> +	(&((struct btf_ptr){.ptr = ptrval, .id = idval}))
>   #endif
[...]
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 7c488a1..f9276f8 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -43,6 +43,7 @@
>   #ifdef CONFIG_BLOCK
>   #include <linux/blkdev.h>
>   #endif
> +#include <linux/btf.h>
>   
>   #include "../mm/internal.h"	/* For the trace_print_flags arrays */
>   
> @@ -2059,6 +2060,103 @@ char *fwnode_string(char *buf, char *end, struct fwnode_handle *fwnode,
>   	return widen_string(buf, buf - buf_start, end, spec);
>   }
>   
> +#if IS_ENABLED(CONFIG_BTF_PRINTF)
> +#define btf_modifier_flag(c)	(c == 'c' ? BTF_SHOW_COMPACT :	\
> +				 c == 'N' ? BTF_SHOW_NONAME :	\
> +				 c == 'x' ? BTF_SHOW_PTR_RAW :	\
> +				 c == '0' ? BTF_SHOW_ZERO : 0)
> +
> +static noinline_for_stack
> +char *btf_string(char *buf, char *end, void *ptr, struct printf_spec spec,
> +		 const char *fmt)
> +{
> +	struct btf_ptr *bp = (struct btf_ptr *)ptr;
> +	u8 btf_kind = BTF_KIND_TYPEDEF;
> +	const struct btf_type *t;
> +	const struct btf *btf;
> +	char *buf_start = buf;
> +	const char *btf_type;
> +	u64 flags = 0, mod;
> +	s32 btf_id;
> +
> +	if (check_pointer(&buf, end, ptr, spec))
> +		return buf;
> +
> +	if (check_pointer(&buf, end, bp->ptr, spec))
> +		return buf;
> +
> +	while (isalnum(*fmt)) {
> +		mod = btf_modifier_flag(*fmt);
> +		if (!mod)
> +			break;
> +		flags |= mod;
> +		fmt++;
> +	}
> +
> +	btf = bpf_get_btf_vmlinux();
> +	if (IS_ERR_OR_NULL(btf))
> +		return ptr_to_id(buf, end, bp->ptr, spec);
> +
> +	if (bp->type != NULL) {
> +		btf_type = bp->type;
> +
> +		if (strncmp(bp->type, "struct ", strlen("struct ")) == 0) {
> +			btf_kind = BTF_KIND_STRUCT;
> +			btf_type += strlen("struct ");
> +		} else if (strncmp(btf_type, "union ", strlen("union ")) == 0) {
> +			btf_kind = BTF_KIND_UNION;
> +			btf_type += strlen("union ");
> +		} else if (strncmp(btf_type, "enum ", strlen("enum ")) == 0) {
> +			btf_kind = BTF_KIND_ENUM;
> +			btf_type += strlen("enum ");
> +		}

I think typedef should be supported here.
In kernel, we have some structure directly defined as typedef's.
A lot of internal int types also typedefs, like u32, atomic_t,
possible_net_t, etc.

A type name without prefix "struct", "union", "enum" can be
treated as a typedef first.

If the type name is not a typedef, it is then compared to a limited
number of C basic int types like "char", "unsigned char", "short",
"unsigned short", ...

> +
> +		if (strlen(btf_type) == 0)
> +			return ptr_to_id(buf, end, bp->ptr, spec);
> +
> +		/*
> +		 * Assume type specified is a typedef as there's not much
> +		 * benefit in specifying int types other than wasting time
> +		 * on BTF lookups; we optimize for the most useful path.
> +		 *
> +		 * Fall back to BTF_KIND_INT if this fails.
> +		 */
> +		btf_id = btf_find_by_name_kind(btf, btf_type, btf_kind);
> +		if (btf_id < 0)
> +			btf_id = btf_find_by_name_kind(btf, btf_type,
> +						       BTF_KIND_INT);
> +	} else if (bp->id > 0)
> +		btf_id = bp->id;
> +	else
> +		return ptr_to_id(buf, end, bp->ptr, spec);
> +
> +	if (btf_id > 0)
> +		t = btf_type_by_id(btf, btf_id);
> +	if (btf_id <= 0 || !t)
> +		return ptr_to_id(buf, end, bp->ptr, spec);
> +
> +	buf += btf_type_snprintf_show(btf, btf_id, bp->ptr, buf,
> +				      end - buf_start, flags);
> +
> +	return widen_string(buf, buf - buf_start, end, spec);
> +}
> +#else
> +static noinline_for_stack
> +char *btf_string(char *buf, char *end, void *ptr, struct printf_spec spec,
> +	const char *fmt)
> +{
> +	struct btf_ptr *bp = (struct btf_ptr *)ptr;
> +
> +	if (check_pointer(&buf, end, ptr, spec))
> +		return buf;
> +
> +	if (check_pointer(&buf, end, bp->ptr, spec))
> +		return buf;
> +
> +	return ptr_to_id(buf, end, bp->ptr, spec);
> +}
> +#endif /* IS_ENABLED(CONFIG_BTF_PRINTF) */
> +
>   /*
>    * Show a '%p' thing.  A kernel extension is that the '%p' is followed
>    * by an extra set of alphanumeric characters that are extended format
> @@ -2169,6 +2267,19 @@ char *fwnode_string(char *buf, char *end, struct fwnode_handle *fwnode,
>    *		P node name, including a possible unit address
>    * - 'x' For printing the address. Equivalent to "%lx".
>    *
> + * - 'T[cNx0]' For printing struct btf_ptr * data using BPF Type Format (BTF).
> + *
> + *			Optional arguments are
> + *			c		compact (no indentation/newlines)
> + *			N		do not print type and member names
> + *			x		do not obfuscate pointers
> + *			0		show 0-valued data
> + *
> + *    BPF_PTR_TYPE(ptr, type) can be used to place pointer and type string
> + *    in the "struct btf_ptr *" expected; for example:
> + *
> + *	printk(KERN_INFO "%pT", BTF_PTR_TYPE(skb, struct sk_buff));
> + *
>    * ** When making changes please also update:
>    *	Documentation/core-api/printk-formats.rst
>    *
> @@ -2251,6 +2362,8 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>   		if (!IS_ERR(ptr))
>   			break;
>   		return err_ptr(buf, end, ptr, spec);
> +	case 'T':
> +		return btf_string(buf, end, ptr, spec, fmt + 1);
>   	}
>   
>   	/* default is to _not_ leak addresses, hash before printing */
> 

Re: [PATCH v2 bpf-next 6/7] bpf: add support for %pT format specifier for bpf_trace_printk() helper

[Yonghong Song]

On 5/11/20 10:56 PM, Alan Maguire wrote:
> Allow %pT[cNx0] format specifier for BTF-based display of data associated
> with pointer.
> 
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---
>   include/uapi/linux/bpf.h       | 27 ++++++++++++++++++++++-----
>   kernel/trace/bpf_trace.c       | 21 ++++++++++++++++++---
>   tools/include/uapi/linux/bpf.h | 27 ++++++++++++++++++++++-----
>   3 files changed, 62 insertions(+), 13 deletions(-)
> 
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 9d1932e..ab3c86c 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -695,7 +695,12 @@ struct bpf_stack_build_id {
>    * 		to file *\/sys/kernel/debug/tracing/trace* from DebugFS, if
>    * 		available. It can take up to three additional **u64**
>    * 		arguments (as an eBPF helpers, the total number of arguments is
> - * 		limited to five).
> + *		limited to five), and also supports %pT (BTF-based type
> + *		printing), as long as BPF_READ lockdown is not active.
> + *		"%pT" takes a "struct __btf_ptr *" as an argument; it
> + *		consists of a pointer value and specified BTF type string or id
> + *		used to select the type for display.  For more details, see
> + *		Documentation/core-api/printk-formats.rst.
>    *
>    * 		Each time the helper is called, it appends a line to the trace.
>    * 		Lines are discarded while *\/sys/kernel/debug/tracing/trace* is
> @@ -731,10 +736,10 @@ struct bpf_stack_build_id {
>    * 		The conversion specifiers supported by *fmt* are similar, but
>    * 		more limited than for printk(). They are **%d**, **%i**,
>    * 		**%u**, **%x**, **%ld**, **%li**, **%lu**, **%lx**, **%lld**,
> - * 		**%lli**, **%llu**, **%llx**, **%p**, **%s**. No modifier (size
> - * 		of field, padding with zeroes, etc.) is available, and the
> - * 		helper will return **-EINVAL** (but print nothing) if it
> - * 		encounters an unknown specifier.
> + *		**%lli**, **%llu**, **%llx**, **%p**, **%pT[cNx0], **%s**.
> + *		Only %pT supports modifiers, and the helper will return
> + *		**-EINVAL** (but print nothing) if it encouters an unknown
> + *		specifier.
>    *
>    * 		Also, note that **bpf_trace_printk**\ () is slow, and should
>    * 		only be used for debugging purposes. For this reason, a notice
> @@ -4058,4 +4063,16 @@ struct bpf_pidns_info {
>   	__u32 pid;
>   	__u32 tgid;
>   };
> +
> +/*
> + * struct __btf_ptr is used for %pT (typed pointer) display; the
> + * additional type string/BTF id are used to render the pointer
> + * data as the appropriate type.
> + */
> +struct __btf_ptr {
> +	void *ptr;
> +	const char *type;
> +	__u32 id;
> +};
> +
>   #endif /* _UAPI__LINUX_BPF_H__ */
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index d961428..c032c58 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -321,9 +321,12 @@ static const struct bpf_func_proto *bpf_get_probe_write_proto(void)
>   	return &bpf_probe_write_user_proto;
>   }
>   
> +#define isbtffmt(c)	\
> +	(c == 'T' || c == 'c' || c == 'N' || c == 'x' || c == '0')
> +
>   /*
>    * Only limited trace_printk() conversion specifiers allowed:
> - * %d %i %u %x %ld %li %lu %lx %lld %lli %llu %llx %p %s
> + * %d %i %u %x %ld %li %lu %lx %lld %lli %llu %llx %p %pT %s
>    */
>   BPF_CALL_5(bpf_trace_printk, char *, fmt, u32, fmt_size, u64, arg1,
>   	   u64, arg2, u64, arg3)
> @@ -361,8 +364,20 @@ static const struct bpf_func_proto *bpf_get_probe_write_proto(void)
>   			i++;
>   		} else if (fmt[i] == 'p' || fmt[i] == 's') {
>   			mod[fmt_cnt]++;
> -			/* disallow any further format extensions */
> -			if (fmt[i + 1] != 0 &&
> +			/*
> +			 * allow BTF type-based printing, and disallow any
> +			 * further format extensions.
> +			 */
> +			if (fmt[i] == 'p' && fmt[i + 1] == 'T') {
> +				int ret;
> +
> +				ret = security_locked_down(LOCKDOWN_BPF_READ);
> +				if (unlikely(ret < 0))
> +					return ret;
> +				i++;
> +				while (isbtffmt(fmt[i]))
> +					i++;

The pointer passed to the helper may not be valid pointer. I think you
need to do a probe_read_kernel() here. Do an atomic memory allocation
here should be okay as this is mostly for debugging only.


> +			} else if (fmt[i + 1] != 0 &&
>   			    !isspace(fmt[i + 1]) &&
>   			    !ispunct(fmt[i + 1]))
>   				return -EINVAL;
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index 9d1932e..ab3c86c 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -695,7 +695,12 @@ struct bpf_stack_build_id {
>    * 		to file *\/sys/kernel/debug/tracing/trace* from DebugFS, if
>    * 		available. It can take up to three additional **u64**
>    * 		arguments (as an eBPF helpers, the total number of arguments is
> - * 		limited to five).
> + *		limited to five), and also supports %pT (BTF-based type
> + *		printing), as long as BPF_READ lockdown is not active.
> + *		"%pT" takes a "struct __btf_ptr *" as an argument; it
> + *		consists of a pointer value and specified BTF type string or id
> + *		used to select the type for display.  For more details, see
> + *		Documentation/core-api/printk-formats.rst.
>    *
>    * 		Each time the helper is called, it appends a line to the trace.
>    * 		Lines are discarded while *\/sys/kernel/debug/tracing/trace* is
> @@ -731,10 +736,10 @@ struct bpf_stack_build_id {
>    * 		The conversion specifiers supported by *fmt* are similar, but
>    * 		more limited than for printk(). They are **%d**, **%i**,
>    * 		**%u**, **%x**, **%ld**, **%li**, **%lu**, **%lx**, **%lld**,
> - * 		**%lli**, **%llu**, **%llx**, **%p**, **%s**. No modifier (size
> - * 		of field, padding with zeroes, etc.) is available, and the
> - * 		helper will return **-EINVAL** (but print nothing) if it
> - * 		encounters an unknown specifier.
> + *		**%lli**, **%llu**, **%llx**, **%p**, **%pT[cNx0], **%s**.
> + *		Only %pT supports modifiers, and the helper will return
> + *		**-EINVAL** (but print nothing) if it encouters an unknown
> + *		specifier.
>    *
>    * 		Also, note that **bpf_trace_printk**\ () is slow, and should
>    * 		only be used for debugging purposes. For this reason, a notice
> @@ -4058,4 +4063,16 @@ struct bpf_pidns_info {
>   	__u32 pid;
>   	__u32 tgid;
>   };
> +
> +/*
> + * struct __btf_ptr is used for %pT (typed pointer) display; the
> + * additional type string/BTF id are used to render the pointer
> + * data as the appropriate type.
> + */
> +struct __btf_ptr {
> +	void *ptr;
> +	const char *type;
> +	__u32 id;
> +};
> +
>   #endif /* _UAPI__LINUX_BPF_H__ */
> 

Re: [PATCH v2 bpf-next 0/7] bpf, printk: add BTF-based type printing

[Alan Maguire]

On Wed, 13 May 2020, Alexei Starovoitov wrote:

> On Tue, May 12, 2020 at 06:56:38AM +0100, Alan Maguire wrote:
> > The printk family of functions support printing specific pointer types
> > using %p format specifiers (MAC addresses, IP addresses, etc).  For
> > full details see Documentation/core-api/printk-formats.rst.
> > 
> > This patchset proposes introducing a "print typed pointer" format
> > specifier "%pT"; the argument associated with the specifier is of
> > form "struct btf_ptr *" which consists of a .ptr value and a .type
> > value specifying a stringified type (e.g. "struct sk_buff") or
> > an .id value specifying a BPF Type Format (BTF) id identifying
> > the appropriate type it points to.
> > 
> > There is already support in kernel/bpf/btf.c for "show" functionality;
> > the changes here generalize that support from seq-file specific
> > verifier display to the more generic case and add another specific
> > use case; snprintf()-style rendering of type information to a
> > provided buffer.  This support is then used to support printk
> > rendering of types, but the intent is to provide a function
> > that might be useful in other in-kernel scenarios; for example:
> > 
> > - ftrace could possibly utilize the function to support typed
> >   display of function arguments by cross-referencing BTF function
> >   information to derive the types of arguments
> > - oops/panic messaging could extend the information displayed to
> >   dig into data structures associated with failing functions
> > 
> > The above potential use cases hint at a potential reply to
> > a reasonable objection that such typed display should be
> > solved by tracing programs, where the in kernel tracing records
> > data and the userspace program prints it out.  While this
> > is certainly the recommended approach for most cases, I
> > believe having an in-kernel mechanism would be valuable
> > also.
> > 
> > The function the printk() family of functions rely on
> > could potentially be used directly for other use cases
> > like ftrace where we might have the BTF ids of the
> > pointers we wish to display; its signature is as follows:
> > 
> > int btf_type_snprintf_show(const struct btf *btf, u32 type_id, void *obj,
> >                            char *buf, int len, u64 flags);
> > 
> > So if ftrace say had the BTF ids of the types of arguments,
> > we see that the above would allow us to convert the
> > pointer data into displayable form.
> > 
> > To give a flavour for what the printed-out data looks like,
> > here we use pr_info() to display a struct sk_buff *.
> > 
> >   struct sk_buff *skb = alloc_skb(64, GFP_KERNEL);
> > 
> >   pr_info("%pT", BTF_PTR_TYPE(skb, "struct sk_buff"));
> > 
> > ...gives us:
> > 
> > (struct sk_buff){
> >  .transport_header = (__u16)65535,
> >  .mac_header = (__u16)65535,
> >  .end = (sk_buff_data_t)192,
> >  .head = (unsigned char *)000000007524fd8b,
> >  .data = (unsigned char *)000000007524fd8b,
> 
> could you add "0x" prefix here to make it even more C like
> and unambiguous ?
>

Sure.
 
> >  .truesize = (unsigned int)768,
> >  .users = (refcount_t){
> >   .refs = (atomic_t){
> >    .counter = (int)1,
> >   },
> >  },
> > }
> > 
> > For bpf_trace_printk() a "struct __btf_ptr *" is used as
> > argument; see tools/testing/selftests/bpf/progs/netif_receive_skb.c
> > for example usage.
> > 
> > The hope is that this functionality will be useful for debugging,
> > and possibly help facilitate the cases mentioned above in the future.
> > 
> > Changes since v1:
> > 
> > - changed format to be more drgn-like, rendering indented type info
> >   along with type names by default (Alexei)
> > - zeroed values are omitted (Arnaldo) by default unless the '0'
> >   modifier is specified (Alexei)
> > - added an option to print pointer values without obfuscation.
> >   The reason to do this is the sysctls controlling pointer display
> >   are likely to be irrelevant in many if not most tracing contexts.
> >   Some questions on this in the outstanding questions section below...
> > - reworked printk format specifer so that we no longer rely on format
> >   %pT<type> but instead use a struct * which contains type information
> >   (Rasmus). This simplifies the printk parsing, makes use more dynamic
> >   and also allows specification by BTF id as well as name.
> > - ensured that BTF-specific printk code is bracketed by
> >   #if ENABLED(CONFIG_BTF_PRINTF)
> 
> I don't like this particular bit:
> +config BTF_PRINTF
> +       bool "print type information using BPF type format"
> +       depends on DEBUG_INFO_BTF
> +       default n
> 
> Looks like it's only purpose is to gate printk test.
> In such case please convert it into hidden config that is
> automatically selected when DEBUG_INFO_BTF is set.
> Or just make printk tests to #if IS_ENABLED(DEBUG_INFO_BTF)
> 

I think the latter makes sense; if BTF isn't there retrieving
vmlinux BTF fails and we simply fall back to standard pointer
printing.  The only part of the code that won't work is the
printk tests that assume BTF display works, and as you suggest
they can just be gated via #if IS_ENABLED(DEBUG_INFO_BTF)

> > - removed incorrect patch which tried to fix dereferencing of resolved
> >   BTF info for vmlinux; instead we skip modifiers for the relevant
> >   case (array element type/size determination) (Alexei).
> > - fixed issues with negative snprintf format length (Rasmus)
> > - added test cases for various data structure formats; base types,
> >   typedefs, structs, etc.
> > - tests now iterate through all typedef, enum, struct and unions
> >   defined for vmlinux BTF and render a version of the target dummy
> >   value which is either all zeros or all 0xff values; the idea is this
> >   exercises the "skip if zero" and "print everything" cases.
> > - added support in BPF for using the %pT format specifier in
> >   bpf_trace_printk()
> > - added BPF tests which ensure %pT format specifier use works (Alexei).
> > 
> > Outstanding issues
> > 
> > - currently %pT is not allowed in BPF programs when lockdown is active
> >   prohibiting BPF_READ; is that sufficient?
> 
> yes. that's enough.
> 
> > - do we want to further restrict the non-obfuscated pointer format
> >   specifier %pTx; for example blocking unprivileged BPF programs from
> >   using it?
> 
> unpriv cannot use bpf_trace_printk() anyway. I'm not sure what is the concern.
> 

I forgot about bpf_trace_printk() not being available.

> > - likely still need a better answer for vmlinux BTF initialization
> >   than the current approach taken; early boot initialization is one
> >   way to go here.
> 
> btf_parse_vmlinux() can certainly be moved to late_initcall().
>

I'll try something in that direction for v3.
 
> > - may be useful to have a "print integers as hex" format modifier (Joe)
> 
> I'd rather stick to the convention that the type drives the way the value is printed.
> For example:
>   u8 ch = 65;
>   pr_info("%pT", BTF_PTR_TYPE(&ch, "char"));
>   prints 'A'
> while
>   u8 ch = 65;
>   pr_info("%pT", BTF_PTR_TYPE(&ch, "u8"));
>   prints 65
> 
> > 
> > Important note: if running test_printf.ko - the version in the bpf-next
> > tree will induce a panic when running the fwnode_pointer() tests due
> > to a kobject issue; applying the patch in
> > 
> > https://lkml.org/lkml/2020/4/17/389
> 
> Thanks for the headsup!
> 
> BTF_PTR_ID() is a great idea as well.
> In the llvm 11 we will introduce __builtin__btf_type_id().
> The bpf program will be able to say:
> bpf_printk("%pT", BTF_PTR_ID(skb, __builtin_btf_type_id(skb, 1)));
> That will help avoid run-time search through btf types by string name.
> The compiler will supply btf_id at build time and libbpf will do
> a relocation into vmlinux btf_id prior to loading.
>

Neat!
 
> Also I think there should be another flag BTF_SHOW_PROBE_DATA.
> It should probe_kernel_read() all data instead of direct dereferences.
> It could be implemented via single probe_kernel_read of the whole BTF data
> structure before pringing the fields one by one. So it should be simple
> addition to patch 2.

Totally agreed we need to prioritize safety for the BPF case.
There's no way to avoid a memory allocation to store the 
probe_kernel_read() data for the BTF_SHOW_PROBE_DATA case is there?
One possible approach would be to DEFINE_PER_CPU() dedicated
buffers; we can observe that because bpf_trace_printk() is
limited in how much data can be displayed, we wouldn't
necessarily need to make such a buffer too big, and could
do multiple probe_kernel_read()s in such cases.

> This flag should be default for printk() from bpf program,
> since the verifier won't be checking that pointer passed into bpf_trace_printk
> is of correct btf type and it's a real pointer.
> Whether this flag should be default for normal printk() is arguable.
> I would make it so. The performance cost of extra copy is small comparing
> with no-crash guarantee it provides. I think it would be a good trade off.
> 

Right, performance isn't the goal here. I think if we could avoid
an allocation, always probe_kernel_read()ing would be best.

> In the future we can extend the verifier so that:
> bpf_safe_printk("%pT", skb);
> will be recognized that 'skb' is PTR_TO_BTF_ID and corresponding and correct
> btf_id is passed into printk. Mistakes will be caught at program
> verification time.
> 

That would be great!

Alan

Re: [PATCH v2 bpf-next 4/7] printk: add type-printing %pT format specifier which uses BTF

[Alan Maguire]

On Wed, 13 May 2020, Yonghong Song wrote:

> 
> 
> On 5/11/20 10:56 PM, Alan Maguire wrote:
> > printk supports multiple pointer object type specifiers (printing
> > netdev features etc).  Extend this support using BTF to cover
> > arbitrary types.  "%pT" specifies the typed format, and the pointer
> > argument is a "struct btf_ptr *" where struct btf_ptr is as follows:
> > 
> > struct btf_ptr {
> >  void *ptr;
> >  const char *type;
> >  u32 id;
> > };
> > 
> > Either the "type" string ("struct sk_buff") or the BTF "id" can be
> > used to identify the type to use in displaying the associated "ptr"
> > value.  A convenience function to create and point at the struct
> > is provided:
> > 
> >  printk(KERN_INFO "%pT", BTF_PTR_TYPE(skb, struct sk_buff));
> > 
> > When invoked, BTF information is used to traverse the sk_buff *
> > and display it.  Support is present for structs, unions, enums,
> > typedefs and core types (though in the latter case there's not
> > much value in using this feature of course).
> > 
> > Default output is indented, but compact output can be specified
> > via the 'c' option.  Type names/member values can be suppressed
> > using the 'N' option.  Zero values are not displayed by default
> > but can be using the '0' option.  Pointer values are obfuscated
> > unless the 'x' option is specified.  As an example:
> > 
> >    struct sk_buff *skb = alloc_skb(64, GFP_KERNEL);
> >    pr_info("%pT", BTF_PTR_TYPE(skb, struct sk_buff));
> > 
> > ...gives us:
> > 
> > (struct sk_buff){
> >   .transport_header = (__u16)65535,
> >   .mac_header = (__u16)65535,
> >   .end = (sk_buff_data_t)192,
> >   .head = (unsigned char *)000000006b71155a,
> >   .data = (unsigned char *)000000006b71155a,
> >   .truesize = (unsigned int)768,
> >   .users = (refcount_t){
> >    .refs = (atomic_t){
> >    .counter = (int)1,
> >   },
> >   },
> >   .extensions = (struct skb_ext *)00000000f486a130,
> > }
> > 
> > printk output is truncated at 1024 bytes.  For cases where overflow
> > is likely, the compact/no type names display modes may be used.
> > 
> > Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> > ---
> >   Documentation/core-api/printk-formats.rst |  15 ++++
> >   include/linux/btf.h                       |   3 +-
> >   include/linux/printk.h                    |  16 +++++
> >   lib/Kconfig                               |  16 +++++
> >   lib/vsprintf.c                            | 113
> >   ++++++++++++++++++++++++++++++
> >   5 files changed, 162 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/core-api/printk-formats.rst
> > b/Documentation/core-api/printk-formats.rst
> > index 8ebe46b1..5c66097 100644
> > --- a/Documentation/core-api/printk-formats.rst
> > +++ b/Documentation/core-api/printk-formats.rst
> > @@ -545,6 +545,21 @@ For printing netdev_features_t.
> >   
> >   Passed by reference.
> >   
> > +BTF-based printing of pointer data
> > +----------------------------------
> > +If '%pT' is specified, use the struct btf_ptr * along with kernel vmlinux
> > +BPF Type Format (BTF) to show the typed data.  For example, specifying
> > +
> > +	printk(KERN_INFO "%pT", BTF_PTR_TYPE(skb, struct_sk_buff));
> > +
> > +will utilize BTF information to traverse the struct sk_buff * and display
> > it.
> > +
> > +Supported modifers are
> > + 'c' compact output (no indentation, newlines etc)
> > + 'N' do not show type names
> > + 'x' show raw pointers (no obfuscation)
> > + '0' show zero-valued data (it is not shown by default)
> > +
> >   Thanks
> >   ======
> >   
> > diff --git a/include/linux/btf.h b/include/linux/btf.h
> > index d571125..7b585ab 100644
> > --- a/include/linux/btf.h
> > +++ b/include/linux/btf.h
> > @@ -169,10 +169,11 @@ static inline const struct btf_member
> > *btf_type_member(const struct btf_type *t)
> >   	return (const struct btf_member *)(t + 1);
> >   }
> >   
> > +struct btf *btf_parse_vmlinux(void);
> > +
> >   #ifdef CONFIG_BPF_SYSCALL
> >   const struct btf_type *btf_type_by_id(const struct btf *btf, u32 type_id);
> >   const char *btf_name_by_offset(const struct btf *btf, u32 offset);
> > -struct btf *btf_parse_vmlinux(void);
> >   struct btf *bpf_prog_get_target_btf(const struct bpf_prog *prog);
> >   #else
> >   static inline const struct btf_type *btf_type_by_id(const struct btf *btf,
> > diff --git a/include/linux/printk.h b/include/linux/printk.h
> > index fcde0772..3c3ea53 100644
> > --- a/include/linux/printk.h
> > +++ b/include/linux/printk.h
> > @@ -528,4 +528,20 @@ static inline void print_hex_dump_debug(const char
> > *prefix_str, int prefix_type,
> >   #define print_hex_dump_bytes(prefix_str, prefix_type, buf, len)	\
> >    print_hex_dump_debug(prefix_str, prefix_type, 16, 1, buf, len, true)
> >   +/**
> > + * struct btf_ptr is used for %pT (typed pointer) display; the
> > + * additional type string/BTF id are used to render the pointer
> > + * data as the appropriate type.
> > + */
> > +struct btf_ptr {
> > +	void *ptr;
> > +	const char *type;
> > +	u32 id;
> > +};
> > +
> > +#define	BTF_PTR_TYPE(ptrval, typeval) \
> > +	(&((struct btf_ptr){.ptr = ptrval, .type = #typeval}))
> > +
> > +#define BTF_PTR_ID(ptrval, idval) \
> > +	(&((struct btf_ptr){.ptr = ptrval, .id = idval}))
> >   #endif
> [...]
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index 7c488a1..f9276f8 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -43,6 +43,7 @@
> >   #ifdef CONFIG_BLOCK
> >   #include <linux/blkdev.h>
> >   #endif
> > +#include <linux/btf.h>
> >   
> >   #include "../mm/internal.h"	/* For the trace_print_flags arrays */
> >   
> > @@ -2059,6 +2060,103 @@ char *fwnode_string(char *buf, char *end, struct
> > fwnode_handle *fwnode,
> >   	return widen_string(buf, buf - buf_start, end, spec);
> >   }
> >   
> > +#if IS_ENABLED(CONFIG_BTF_PRINTF)
> > +#define btf_modifier_flag(c)	(c == 'c' ? BTF_SHOW_COMPACT :	\
> > +				 c == 'N' ? BTF_SHOW_NONAME :	\
> > +				 c == 'x' ? BTF_SHOW_PTR_RAW :	\
> > +				 c == '0' ? BTF_SHOW_ZERO : 0)
> > +
> > +static noinline_for_stack
> > +char *btf_string(char *buf, char *end, void *ptr, struct printf_spec spec,
> > +		 const char *fmt)
> > +{
> > +	struct btf_ptr *bp = (struct btf_ptr *)ptr;
> > +	u8 btf_kind = BTF_KIND_TYPEDEF;
> > +	const struct btf_type *t;
> > +	const struct btf *btf;
> > +	char *buf_start = buf;
> > +	const char *btf_type;
> > +	u64 flags = 0, mod;
> > +	s32 btf_id;
> > +
> > +	if (check_pointer(&buf, end, ptr, spec))
> > +		return buf;
> > +
> > +	if (check_pointer(&buf, end, bp->ptr, spec))
> > +		return buf;
> > +
> > +	while (isalnum(*fmt)) {
> > +		mod = btf_modifier_flag(*fmt);
> > +		if (!mod)
> > +			break;
> > +		flags |= mod;
> > +		fmt++;
> > +	}
> > +
> > +	btf = bpf_get_btf_vmlinux();
> > +	if (IS_ERR_OR_NULL(btf))
> > +		return ptr_to_id(buf, end, bp->ptr, spec);
> > +
> > +	if (bp->type != NULL) {
> > +		btf_type = bp->type;
> > +
> > +		if (strncmp(bp->type, "struct ", strlen("struct ")) == 0) {
> > +			btf_kind = BTF_KIND_STRUCT;
> > +			btf_type += strlen("struct ");
> > +		} else if (strncmp(btf_type, "union ", strlen("union ")) == 0)
> > {
> > +			btf_kind = BTF_KIND_UNION;
> > +			btf_type += strlen("union ");
> > +		} else if (strncmp(btf_type, "enum ", strlen("enum ")) == 0) {
> > +			btf_kind = BTF_KIND_ENUM;
> > +			btf_type += strlen("enum ");
> > +		}
> 
> I think typedef should be supported here.
> In kernel, we have some structure directly defined as typedef's.
> A lot of internal int types also typedefs, like u32, atomic_t,
> possible_net_t, etc.
> 
> A type name without prefix "struct", "union", "enum" can be
> treated as a typedef first.
> 

That's how the code works today; we start with a typedef assumption.
See the comment below starting "Assume type specified is a typedef";
we initialize btf_kind to be a typedef above; it's only changed
to an BTF_KIND_INT if we find a struct/enum/union prefix or if lookup
using the typedef kind fails. I should probably make this clearer
though (move the comment up maybe?). Thanks for taking a look!

> If the type name is not a typedef, it is then compared to a limited
> number of C basic int types like "char", "unsigned char", "short",
> "unsigned short", ...
> 
> > +
> > +		if (strlen(btf_type) == 0)
> > +			return ptr_to_id(buf, end, bp->ptr, spec);
> > +
> > +		/*
> > +		 * Assume type specified is a typedef as there's not much
> > +		 * benefit in specifying int types other than wasting time
> > +		 * on BTF lookups; we optimize for the most useful path.
> > +		 *
> > +		 * Fall back to BTF_KIND_INT if this fails.
> > +		 */
> > +		btf_id = btf_find_by_name_kind(btf, btf_type, btf_kind);
> > +		if (btf_id < 0)
> > +			btf_id = btf_find_by_name_kind(btf, btf_type,
> > +						       BTF_KIND_INT);
> > +	} else if (bp->id > 0)
> > +		btf_id = bp->id;
> > +	else
> > +		return ptr_to_id(buf, end, bp->ptr, spec);
> > +
> > +	if (btf_id > 0)
> > +		t = btf_type_by_id(btf, btf_id);
> > +	if (btf_id <= 0 || !t)
> > +		return ptr_to_id(buf, end, bp->ptr, spec);
> > +
> > +	buf += btf_type_snprintf_show(btf, btf_id, bp->ptr, buf,
> > +				      end - buf_start, flags);
> > +
> > +	return widen_string(buf, buf - buf_start, end, spec);
> > +}
> > +#else
> > +static noinline_for_stack
> > +char *btf_string(char *buf, char *end, void *ptr, struct printf_spec spec,
> > +	const char *fmt)
> > +{
> > +	struct btf_ptr *bp = (struct btf_ptr *)ptr;
> > +
> > +	if (check_pointer(&buf, end, ptr, spec))
> > +		return buf;
> > +
> > +	if (check_pointer(&buf, end, bp->ptr, spec))
> > +		return buf;
> > +
> > +	return ptr_to_id(buf, end, bp->ptr, spec);
> > +}
> > +#endif /* IS_ENABLED(CONFIG_BTF_PRINTF) */
> > +
> >   /*
> >    * Show a '%p' thing.  A kernel extension is that the '%p' is followed
> >    * by an extra set of alphanumeric characters that are extended format
> > @@ -2169,6 +2267,19 @@ char *fwnode_string(char *buf, char *end, struct
> > fwnode_handle *fwnode,
> >    *		P node name, including a possible unit address
> >    * - 'x' For printing the address. Equivalent to "%lx".
> >    *
> > + * - 'T[cNx0]' For printing struct btf_ptr * data using BPF Type Format
> > (BTF).
> > + *
> > + *			Optional arguments are
> > + *			c		compact (no indentation/newlines)
> > + *			N		do not print type and member names
> > + *			x		do not obfuscate pointers
> > + *			0		show 0-valued data
> > + *
> > + *    BPF_PTR_TYPE(ptr, type) can be used to place pointer and type string
> > + *    in the "struct btf_ptr *" expected; for example:
> > + *
> > + *	printk(KERN_INFO "%pT", BTF_PTR_TYPE(skb, struct sk_buff));
> > + *
> >    * ** When making changes please also update:
> >    *	Documentation/core-api/printk-formats.rst
> >    *
> > @@ -2251,6 +2362,8 @@ char *pointer(const char *fmt, char *buf, char *end,
> > void *ptr,
> >     if (!IS_ERR(ptr))
> >     	break;
> >   		return err_ptr(buf, end, ptr, spec);
> > +	case 'T':
> > +		return btf_string(buf, end, ptr, spec, fmt + 1);
> >    }
> >   
> >    /* default is to _not_ leak addresses, hash before printing */
> > 
> 
> 

Re: [PATCH v2 bpf-next 4/7] printk: add type-printing %pT format specifier which uses BTF

[Joe Perches]

On Wed, 2020-05-13 at 16:22 -0700, Joe Perches wrote:
> On Wed, 2020-05-13 at 16:07 -0700, Alexei Starovoitov wrote:
> > On Wed, May 13, 2020 at 4:05 PM Joe Perches <joe@perches.com> wrote:
> > > On Tue, 2020-05-12 at 06:56 +0100, Alan Maguire wrote:
> > > > printk supports multiple pointer object type specifiers (printing
> > > > netdev features etc).  Extend this support using BTF to cover
> > > > arbitrary types.  "%pT" specifies the typed format, and the pointer
> > > > argument is a "struct btf_ptr *" where struct btf_ptr is as follows:
> > > > 
> > > > struct btf_ptr {
> > > >       void *ptr;
> > > >       const char *type;
> > > >       u32 id;
> > > > };
> > > > 
> > > > Either the "type" string ("struct sk_buff") or the BTF "id" can be
> > > > used to identify the type to use in displaying the associated "ptr"
> > > > value.  A convenience function to create and point at the struct
> > > > is provided:
> > > > 
> > > >       printk(KERN_INFO "%pT", BTF_PTR_TYPE(skb, struct sk_buff));
> > > > 
> > > > When invoked, BTF information is used to traverse the sk_buff *
> > > > and display it.  Support is present for structs, unions, enums,
> > > > typedefs and core types (though in the latter case there's not
> > > > much value in using this feature of course).
> > > > 
> > > > Default output is indented, but compact output can be specified
> > > > via the 'c' option.  Type names/member values can be suppressed
> > > > using the 'N' option.  Zero values are not displayed by default
> > > > but can be using the '0' option.  Pointer values are obfuscated
> > > > unless the 'x' option is specified.  As an example:
> > > > 
> > > >   struct sk_buff *skb = alloc_skb(64, GFP_KERNEL);
> > > >   pr_info("%pT", BTF_PTR_TYPE(skb, struct sk_buff));
> > > > 
> > > > ...gives us:
> > > > 
> > > > (struct sk_buff){
> > > >  .transport_header = (__u16)65535,
> > > >        .mac_header = (__u16)65535,
> > > >  .end = (sk_buff_data_t)192,
> > > >  .head = (unsigned char *)000000006b71155a,
> > > >  .data = (unsigned char *)000000006b71155a,
> > > >  .truesize = (unsigned int)768,
> > > >  .users = (refcount_t){
> > > >   .refs = (atomic_t){
> > > >    .counter = (int)1,
> > > 
> > > Given
> > > 
> > >   #define BTF_INT_ENCODING(VAL)   (((VAL) & 0x0f000000) >> 24)
> > > 
> > > Maybe
> > > 
> > >   #define BTF_INT_SIGNED  (1 << 0)
> > >   #define BTF_INT_CHAR    (1 << 1)
> > >   #define BTF_INT_BOOL    (1 << 2)
> > > 
> > > could be extended to include
> > > 
> > >   #define BTF_INT_HEX     (1 << 3)
> > > 
> > > So hex values can be appropriately pretty-printed.
> > 
> > Nack to that.
> 
> why?
> 

Tell me what's wrong with the idea.

Here's a possible implementation:
---
 Documentation/bpf/btf.rst      |  5 +++--
 include/uapi/linux/btf.h       |  1 +
 kernel/bpf/btf.c               |  5 ++++-
 tools/bpf/bpftool/btf.c        |  2 ++
 tools/bpf/bpftool/btf_dumper.c | 13 +++++++++++++
 tools/include/uapi/linux/btf.h |  1 +
 6 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/Documentation/bpf/btf.rst b/Documentation/bpf/btf.rst
index 4d565d202ce3..56aaa189e7fb 100644
--- a/Documentation/bpf/btf.rst
+++ b/Documentation/bpf/btf.rst
@@ -139,10 +139,11 @@ The ``BTF_INT_ENCODING`` has the following attributes::
   #define BTF_INT_SIGNED  (1 << 0)
   #define BTF_INT_CHAR    (1 << 1)
   #define BTF_INT_BOOL    (1 << 2)
+  #define BTF_INT_HEX     (1 << 3)
 
 The ``BTF_INT_ENCODING()`` provides extra information: signedness, char, or
-bool, for the int type. The char and bool encoding are mostly useful for
-pretty print. At most one encoding can be specified for the int type.
+bool, for the int type. The char, bool and hex encodings are mostly useful
+for pretty print. At most one encoding can be specified for the int type.
 
 The ``BTF_INT_BITS()`` specifies the number of actual bits held by this int
 type. For example, a 4-bit bitfield encodes ``BTF_INT_BITS()`` equals to 4.
diff --git a/include/uapi/linux/btf.h b/include/uapi/linux/btf.h
index 5a667107ad2c..36f309209786 100644
--- a/include/uapi/linux/btf.h
+++ b/include/uapi/linux/btf.h
@@ -90,6 +90,7 @@ struct btf_type {
 #define BTF_INT_SIGNED	(1 << 0)
 #define BTF_INT_CHAR	(1 << 1)
 #define BTF_INT_BOOL	(1 << 2)
+#define BTF_INT_HEX	(1 << 3)
 
 /* BTF_KIND_ENUM is followed by multiple "struct btf_enum".
  * The exact number of btf_enum is stored in the vlen (of the
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 58c9af1d4808..90bdc0635321 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -501,6 +501,8 @@ static const char *btf_int_encoding_str(u8 encoding)
 		return "CHAR";
 	else if (encoding == BTF_INT_BOOL)
 		return "BOOL";
+	else if (encoding == BTF_INT_HEX)
+		return "HEX";
 	else
 		return "UNKN";
 }
@@ -1404,7 +1406,8 @@ static s32 btf_int_check_meta(struct btf_verifier_env *env,
 	if (encoding &&
 	    encoding != BTF_INT_SIGNED &&
 	    encoding != BTF_INT_CHAR &&
-	    encoding != BTF_INT_BOOL) {
+	    encoding != BTF_INT_BOOL &&
+	    encoding != BTF_INT_HEX) {
 		btf_verifier_log_type(env, t, "Unsupported encoding");
 		return -ENOTSUPP;
 	}
diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
index 41a1346934a1..44a129c40873 100644
--- a/tools/bpf/bpftool/btf.c
+++ b/tools/bpf/bpftool/btf.c
@@ -59,6 +59,8 @@ static const char *btf_int_enc_str(__u8 encoding)
 		return "CHAR";
 	case BTF_INT_BOOL:
 		return "BOOL";
+	case BTF_INT_HEX:
+		return "HEX";
 	default:
 		return "UNKN";
 	}
diff --git a/tools/bpf/bpftool/btf_dumper.c b/tools/bpf/bpftool/btf_dumper.c
index ede162f83eea..96947ef92565 100644
--- a/tools/bpf/bpftool/btf_dumper.c
+++ b/tools/bpf/bpftool/btf_dumper.c
@@ -418,6 +418,19 @@ static int btf_dumper_int(const struct btf_type *t, __u8 bit_offset,
 	case BTF_INT_BOOL:
 		jsonw_bool(jw, *(int *)data);
 		break;
+	case BTF_INT_HEX:
+		if (BTF_INT_BITS(*int_type) == 64)
+			jsonw_printf(jw, "%llx", *(long long *)data);
+		else if (BTF_INT_BITS(*int_type) == 32)
+			jsonw_printf(jw, "%x", *(int *)data);
+		else if (BTF_INT_BITS(*int_type) == 16)
+			jsonw_printf(jw, "%hx", *(short *)data);
+		else if (BTF_INT_BITS(*int_type) == 8)
+			jsonw_printf(jw, "%hhx", *(char *)data);
+		else
+			btf_dumper_int_bits(*int_type, bit_offset, data, jw,
+					    is_plain_text);
+		break;
 	default:
 		/* shouldn't happen */
 		return -EINVAL;
diff --git a/tools/include/uapi/linux/btf.h b/tools/include/uapi/linux/btf.h
index 5a667107ad2c..36f309209786 100644
--- a/tools/include/uapi/linux/btf.h
+++ b/tools/include/uapi/linux/btf.h
@@ -90,6 +90,7 @@ struct btf_type {
 #define BTF_INT_SIGNED	(1 << 0)
 #define BTF_INT_CHAR	(1 << 1)
 #define BTF_INT_BOOL	(1 << 2)
+#define BTF_INT_HEX	(1 << 3)
 
 /* BTF_KIND_ENUM is followed by multiple "struct btf_enum".
  * The exact number of btf_enum is stored in the vlen (of the

Re: [PATCH v2 bpf-next 4/7] printk: add type-printing %pT format specifier which uses BTF

[Alexei Starovoitov]

On Thu, May 14, 2020 at 04:43:24PM -0700, Joe Perches wrote:
>  The ``BTF_INT_BITS()`` specifies the number of actual bits held by this int
>  type. For example, a 4-bit bitfield encodes ``BTF_INT_BITS()`` equals to 4.
> diff --git a/include/uapi/linux/btf.h b/include/uapi/linux/btf.h
> index 5a667107ad2c..36f309209786 100644
> --- a/include/uapi/linux/btf.h
> +++ b/include/uapi/linux/btf.h
> @@ -90,6 +90,7 @@ struct btf_type {
>  #define BTF_INT_SIGNED	(1 << 0)
>  #define BTF_INT_CHAR	(1 << 1)
>  #define BTF_INT_BOOL	(1 << 2)
> +#define BTF_INT_HEX	(1 << 3)

Nack.
Hex is not a type.

Re: [PATCH v2 bpf-next 4/7] printk: add type-printing %pT format specifier which uses BTF

[Joe Perches]

On Thu, 2020-05-14 at 17:09 -0700, Alexei Starovoitov wrote:
> On Thu, May 14, 2020 at 04:43:24PM -0700, Joe Perches wrote:
> >  The ``BTF_INT_BITS()`` specifies the number of actual bits held by this int
> >  type. For example, a 4-bit bitfield encodes ``BTF_INT_BITS()`` equals to 4.
> > diff --git a/include/uapi/linux/btf.h b/include/uapi/linux/btf.h
> > index 5a667107ad2c..36f309209786 100644
> > --- a/include/uapi/linux/btf.h
> > +++ b/include/uapi/linux/btf.h
> > @@ -90,6 +90,7 @@ struct btf_type {
> >  #define BTF_INT_SIGNED	(1 << 0)
> >  #define BTF_INT_CHAR	(1 << 1)
> >  #define BTF_INT_BOOL	(1 << 2)
> > +#define BTF_INT_HEX	(1 << 3)
> 
> Nack.
> Hex is not a type.

It is a pretty print output format.

Re: [PATCH v2 bpf-next 7/7] bpf: add tests for %pT format specifier

[Andrii Nakryiko]

On Mon, May 11, 2020 at 10:59 PM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> tests verify we get > 0 return value from bpf_trace_print()
> using %pT format specifier with various modifiers/pointer
> values.
>
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---

There is no need to use perf buffer for returning results to
user-space. See prog_tests/skeleton.c and progs/test_skeleton.c for a
very minimalistic and simple way to do tests like this.

>  .../selftests/bpf/prog_tests/trace_printk_btf.c    | 83 ++++++++++++++++++++++
>  .../selftests/bpf/progs/netif_receive_skb.c        | 81 +++++++++++++++++++++
>  2 files changed, 164 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/trace_printk_btf.c
>  create mode 100644 tools/testing/selftests/bpf/progs/netif_receive_skb.c

[...]

> diff --git a/tools/testing/selftests/bpf/progs/netif_receive_skb.c b/tools/testing/selftests/bpf/progs/netif_receive_skb.c
> new file mode 100644
> index 0000000..b5148df
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/netif_receive_skb.c
> @@ -0,0 +1,81 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2020, Oracle and/or its affiliates. */
> +#include <linux/bpf.h>
> +#include <stdbool.h>
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_endian.h>
> +#include <bpf/bpf_tracing.h>
> +
> +char _license[] SEC("license") = "GPL";
> +
> +struct {
> +       __uint(type, BPF_MAP_TYPE_PERF_EVENT_ARRAY);
> +       __uint(key_size, sizeof(int));
> +       __uint(value_size, sizeof(int));
> +} perf_buf_map SEC(".maps");
> +
> +struct result {
> +       int ret;
> +       int subtest;
> +       int num_subtest;
> +};
> +
> +typedef struct {
> +       int counter;
> +} atomic_t;
> +typedef struct refcount_struct {
> +       atomic_t refs;
> +} refcount_t;
> +
> +struct sk_buff {
> +       /* field names and sizes should match to those in the kernel */
> +       unsigned int len, data_len;
> +       __u16 mac_len, hdr_len, queue_mapping;
> +       struct net_device *dev;
> +       /* order of the fields doesn't matter */
> +       refcount_t users;
> +       unsigned char *data;
> +       char __pkt_type_offset[0];
> +       char cb[48];
> +};


please use vmlinux.h instead of duplicating these definitions (which
also will start failing, when sk_buff definition will change).

> +
> +#define CHECK_PRINTK(_fmt, _p, res)                                    \
> +       do {                                                            \
> +               char fmt[] = _fmt;                                      \
> +               ++(res)->num_subtest;                                   \
> +               if ((res)->ret >= 0) {                                  \
> +                       ++(res)->subtest;                               \
> +                       (res)->ret = bpf_trace_printk(fmt, sizeof(fmt), \
> +                                                     (_p));            \
> +               }                                                       \
> +       } while (0)
> +
> +/* TRACE_EVENT(netif_receive_skb,
> + *     TP_PROTO(struct sk_buff *skb),
> + */
> +SEC("tp_btf/netif_receive_skb")
> +int BPF_PROG(trace_netif_receive_skb, struct sk_buff *skb)
> +{
> +       char skb_type[] = "struct sk_buff";
> +       struct __btf_ptr nullp = { .ptr = 0, .type = skb_type };
> +       struct __btf_ptr p = { .ptr = skb, .type = skb_type };
> +       struct result res = { 0, 0 };
> +
> +       CHECK_PRINTK("%pT\n", &p, &res);
> +       CHECK_PRINTK("%pTc\n", &p, &res);
> +       CHECK_PRINTK("%pTN\n", &p, &res);
> +       CHECK_PRINTK("%pTx\n", &p, &res);
> +       CHECK_PRINTK("%pT0\n", &p, &res);
> +       CHECK_PRINTK("%pTcNx0\n", &p, &res);
> +       CHECK_PRINTK("%pT\n", &nullp, &res);
> +       CHECK_PRINTK("%pTc\n", &nullp, &res);
> +       CHECK_PRINTK("%pTN\n", &nullp, &res);
> +       CHECK_PRINTK("%pTx\n", &nullp, &res);
> +       CHECK_PRINTK("%pT0\n", &nullp, &res);
> +       CHECK_PRINTK("%pTcNx0\n", &nullp, &res);

with global variables this would be:

int pT = 0;
int pTc = 0;
/* and so on */

then inside BPF_PROG:

pT = bpf_printk("%pT\n", &p);
pTc = bpf_printk("%pTc\n", &p);
/* and so on */

CHECK_PRINTK isn't necessary, IMO. bpf_printk is defined in bpf_helpers.h

> +
> +       bpf_perf_event_output(ctx, &perf_buf_map, BPF_F_CURRENT_CPU,
> +                             &res, sizeof(res));
> +
> +       return 0;
> +}
> --
> 1.8.3.1
>

Re: [PATCH v2 bpf-next 4/7] printk: add type-printing %pT format specifier which uses BTF

[Yonghong Song]

On 5/14/20 3:37 PM, Alan Maguire wrote:
> 
> 
> On Wed, 13 May 2020, Yonghong Song wrote:
> 
>>
>>
>> On 5/11/20 10:56 PM, Alan Maguire wrote:
>>> printk supports multiple pointer object type specifiers (printing
>>> netdev features etc).  Extend this support using BTF to cover
>>> arbitrary types.  "%pT" specifies the typed format, and the pointer
>>> argument is a "struct btf_ptr *" where struct btf_ptr is as follows:
>>>
>>> struct btf_ptr {
>>>   void *ptr;
>>>   const char *type;
>>>   u32 id;
>>> };
>>>
>>> Either the "type" string ("struct sk_buff") or the BTF "id" can be
>>> used to identify the type to use in displaying the associated "ptr"
>>> value.  A convenience function to create and point at the struct
>>> is provided:
>>>
>>>   printk(KERN_INFO "%pT", BTF_PTR_TYPE(skb, struct sk_buff));
>>>
>>> When invoked, BTF information is used to traverse the sk_buff *
>>> and display it.  Support is present for structs, unions, enums,
>>> typedefs and core types (though in the latter case there's not
>>> much value in using this feature of course).
>>>
>>> Default output is indented, but compact output can be specified
>>> via the 'c' option.  Type names/member values can be suppressed
>>> using the 'N' option.  Zero values are not displayed by default
>>> but can be using the '0' option.  Pointer values are obfuscated
>>> unless the 'x' option is specified.  As an example:
>>>
>>>     struct sk_buff *skb = alloc_skb(64, GFP_KERNEL);
>>>     pr_info("%pT", BTF_PTR_TYPE(skb, struct sk_buff));
>>>
>>> ...gives us:
>>>
>>> (struct sk_buff){
>>>    .transport_header = (__u16)65535,
>>>    .mac_header = (__u16)65535,
>>>    .end = (sk_buff_data_t)192,
>>>    .head = (unsigned char *)000000006b71155a,
>>>    .data = (unsigned char *)000000006b71155a,
>>>    .truesize = (unsigned int)768,
>>>    .users = (refcount_t){
>>>     .refs = (atomic_t){
>>>     .counter = (int)1,
>>>    },
>>>    },
>>>    .extensions = (struct skb_ext *)00000000f486a130,
>>> }
>>>
>>> printk output is truncated at 1024 bytes.  For cases where overflow
>>> is likely, the compact/no type names display modes may be used.
>>>
>>> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
>>> ---
>>>    Documentation/core-api/printk-formats.rst |  15 ++++
>>>    include/linux/btf.h                       |   3 +-
>>>    include/linux/printk.h                    |  16 +++++
>>>    lib/Kconfig                               |  16 +++++
>>>    lib/vsprintf.c                            | 113
>>>    ++++++++++++++++++++++++++++++
>>>    5 files changed, 162 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/core-api/printk-formats.rst
>>> b/Documentation/core-api/printk-formats.rst
>>> index 8ebe46b1..5c66097 100644
>>> --- a/Documentation/core-api/printk-formats.rst
>>> +++ b/Documentation/core-api/printk-formats.rst
>>> @@ -545,6 +545,21 @@ For printing netdev_features_t.
>>>    
>>>    Passed by reference.
>>>    
>>> +BTF-based printing of pointer data
>>> +----------------------------------
>>> +If '%pT' is specified, use the struct btf_ptr * along with kernel vmlinux
>>> +BPF Type Format (BTF) to show the typed data.  For example, specifying
>>> +
>>> +	printk(KERN_INFO "%pT", BTF_PTR_TYPE(skb, struct_sk_buff));
>>> +
>>> +will utilize BTF information to traverse the struct sk_buff * and display
>>> it.
>>> +
>>> +Supported modifers are
>>> + 'c' compact output (no indentation, newlines etc)
>>> + 'N' do not show type names
>>> + 'x' show raw pointers (no obfuscation)
>>> + '0' show zero-valued data (it is not shown by default)
>>> +
>>>    Thanks
>>>    ======
>>>    
>>> diff --git a/include/linux/btf.h b/include/linux/btf.h
>>> index d571125..7b585ab 100644
>>> --- a/include/linux/btf.h
>>> +++ b/include/linux/btf.h
>>> @@ -169,10 +169,11 @@ static inline const struct btf_member
>>> *btf_type_member(const struct btf_type *t)
>>>    	return (const struct btf_member *)(t + 1);
>>>    }
>>>    
>>> +struct btf *btf_parse_vmlinux(void);
>>> +
>>>    #ifdef CONFIG_BPF_SYSCALL
>>>    const struct btf_type *btf_type_by_id(const struct btf *btf, u32 type_id);
>>>    const char *btf_name_by_offset(const struct btf *btf, u32 offset);
>>> -struct btf *btf_parse_vmlinux(void);
>>>    struct btf *bpf_prog_get_target_btf(const struct bpf_prog *prog);
>>>    #else
>>>    static inline const struct btf_type *btf_type_by_id(const struct btf *btf,
>>> diff --git a/include/linux/printk.h b/include/linux/printk.h
>>> index fcde0772..3c3ea53 100644
>>> --- a/include/linux/printk.h
>>> +++ b/include/linux/printk.h
>>> @@ -528,4 +528,20 @@ static inline void print_hex_dump_debug(const char
>>> *prefix_str, int prefix_type,
>>>    #define print_hex_dump_bytes(prefix_str, prefix_type, buf, len)	\
>>>     print_hex_dump_debug(prefix_str, prefix_type, 16, 1, buf, len, true)
>>>    +/**
>>> + * struct btf_ptr is used for %pT (typed pointer) display; the
>>> + * additional type string/BTF id are used to render the pointer
>>> + * data as the appropriate type.
>>> + */
>>> +struct btf_ptr {
>>> +	void *ptr;
>>> +	const char *type;
>>> +	u32 id;
>>> +};
>>> +
>>> +#define	BTF_PTR_TYPE(ptrval, typeval) \
>>> +	(&((struct btf_ptr){.ptr = ptrval, .type = #typeval}))
>>> +
>>> +#define BTF_PTR_ID(ptrval, idval) \
>>> +	(&((struct btf_ptr){.ptr = ptrval, .id = idval}))
>>>    #endif
>> [...]
>>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
>>> index 7c488a1..f9276f8 100644
>>> --- a/lib/vsprintf.c
>>> +++ b/lib/vsprintf.c
>>> @@ -43,6 +43,7 @@
>>>    #ifdef CONFIG_BLOCK
>>>    #include <linux/blkdev.h>
>>>    #endif
>>> +#include <linux/btf.h>
>>>    
>>>    #include "../mm/internal.h"	/* For the trace_print_flags arrays */
>>>    
>>> @@ -2059,6 +2060,103 @@ char *fwnode_string(char *buf, char *end, struct
>>> fwnode_handle *fwnode,
>>>    	return widen_string(buf, buf - buf_start, end, spec);
>>>    }
>>>    
>>> +#if IS_ENABLED(CONFIG_BTF_PRINTF)
>>> +#define btf_modifier_flag(c)	(c == 'c' ? BTF_SHOW_COMPACT :	\
>>> +				 c == 'N' ? BTF_SHOW_NONAME :	\
>>> +				 c == 'x' ? BTF_SHOW_PTR_RAW :	\
>>> +				 c == '0' ? BTF_SHOW_ZERO : 0)
>>> +
>>> +static noinline_for_stack
>>> +char *btf_string(char *buf, char *end, void *ptr, struct printf_spec spec,
>>> +		 const char *fmt)
>>> +{
>>> +	struct btf_ptr *bp = (struct btf_ptr *)ptr;
>>> +	u8 btf_kind = BTF_KIND_TYPEDEF;
>>> +	const struct btf_type *t;
>>> +	const struct btf *btf;
>>> +	char *buf_start = buf;
>>> +	const char *btf_type;
>>> +	u64 flags = 0, mod;
>>> +	s32 btf_id;
>>> +
>>> +	if (check_pointer(&buf, end, ptr, spec))
>>> +		return buf;
>>> +
>>> +	if (check_pointer(&buf, end, bp->ptr, spec))
>>> +		return buf;
>>> +
>>> +	while (isalnum(*fmt)) {
>>> +		mod = btf_modifier_flag(*fmt);
>>> +		if (!mod)
>>> +			break;
>>> +		flags |= mod;
>>> +		fmt++;
>>> +	}
>>> +
>>> +	btf = bpf_get_btf_vmlinux();
>>> +	if (IS_ERR_OR_NULL(btf))
>>> +		return ptr_to_id(buf, end, bp->ptr, spec);
>>> +
>>> +	if (bp->type != NULL) {
>>> +		btf_type = bp->type;
>>> +
>>> +		if (strncmp(bp->type, "struct ", strlen("struct ")) == 0) {
>>> +			btf_kind = BTF_KIND_STRUCT;
>>> +			btf_type += strlen("struct ");
>>> +		} else if (strncmp(btf_type, "union ", strlen("union ")) == 0)
>>> {
>>> +			btf_kind = BTF_KIND_UNION;
>>> +			btf_type += strlen("union ");
>>> +		} else if (strncmp(btf_type, "enum ", strlen("enum ")) == 0) {
>>> +			btf_kind = BTF_KIND_ENUM;
>>> +			btf_type += strlen("enum ");
>>> +		}
>>
>> I think typedef should be supported here.
>> In kernel, we have some structure directly defined as typedef's.
>> A lot of internal int types also typedefs, like u32, atomic_t,
>> possible_net_t, etc.
>>
>> A type name without prefix "struct", "union", "enum" can be
>> treated as a typedef first.
>>
> 
> That's how the code works today; we start with a typedef assumption.
> See the comment below starting "Assume type specified is a typedef";
> we initialize btf_kind to be a typedef above; it's only changed
> to an BTF_KIND_INT if we find a struct/enum/union prefix or if lookup
> using the typedef kind fails. I should probably make this clearer
> though (move the comment up maybe?). Thanks for taking a look!

Thanks for explanation. I missed it. Move the comments up about what to 
support explicitly will be good.

> 
>> If the type name is not a typedef, it is then compared to a limited
>> number of C basic int types like "char", "unsigned char", "short",
>> "unsigned short", ...
>>
>>> +
>>> +		if (strlen(btf_type) == 0)
>>> +			return ptr_to_id(buf, end, bp->ptr, spec);
>>> +
>>> +		/*
>>> +		 * Assume type specified is a typedef as there's not much
>>> +		 * benefit in specifying int types other than wasting time
>>> +		 * on BTF lookups; we optimize for the most useful path.
>>> +		 *
>>> +		 * Fall back to BTF_KIND_INT if this fails.
>>> +		 */
>>> +		btf_id = btf_find_by_name_kind(btf, btf_type, btf_kind);
>>> +		if (btf_id < 0)
>>> +			btf_id = btf_find_by_name_kind(btf, btf_type,
>>> +						       BTF_KIND_INT);
>>> +	} else if (bp->id > 0)
>>> +		btf_id = bp->id;
>>> +	else
>>> +		return ptr_to_id(buf, end, bp->ptr, spec);
>>> +
>>> +	if (btf_id > 0)
>>> +		t = btf_type_by_id(btf, btf_id);
>>> +	if (btf_id <= 0 || !t)
>>> +		return ptr_to_id(buf, end, bp->ptr, spec);
>>> +
>>> +	buf += btf_type_snprintf_show(btf, btf_id, bp->ptr, buf,
>>> +				      end - buf_start, flags);
>>> +
>>> +	return widen_string(buf, buf - buf_start, end, spec);
>>> +}
[...]

Re: [PATCH v2 bpf-next 0/7] bpf, printk: add BTF-based type printing

[Yonghong Song]

On 5/13/20 3:24 PM, Alexei Starovoitov wrote:
> On Tue, May 12, 2020 at 06:56:38AM +0100, Alan Maguire wrote:
>> The printk family of functions support printing specific pointer types
>> using %p format specifiers (MAC addresses, IP addresses, etc).  For
>> full details see Documentation/core-api/printk-formats.rst.
>>
>> This patchset proposes introducing a "print typed pointer" format
>> specifier "%pT"; the argument associated with the specifier is of
>> form "struct btf_ptr *" which consists of a .ptr value and a .type
>> value specifying a stringified type (e.g. "struct sk_buff") or
>> an .id value specifying a BPF Type Format (BTF) id identifying
>> the appropriate type it points to.
>>
>> There is already support in kernel/bpf/btf.c for "show" functionality;
>> the changes here generalize that support from seq-file specific
>> verifier display to the more generic case and add another specific
>> use case; snprintf()-style rendering of type information to a
>> provided buffer.  This support is then used to support printk
>> rendering of types, but the intent is to provide a function
>> that might be useful in other in-kernel scenarios; for example:
>>
>> - ftrace could possibly utilize the function to support typed
>>    display of function arguments by cross-referencing BTF function
>>    information to derive the types of arguments
>> - oops/panic messaging could extend the information displayed to
>>    dig into data structures associated with failing functions
>>
>> The above potential use cases hint at a potential reply to
>> a reasonable objection that such typed display should be
>> solved by tracing programs, where the in kernel tracing records
>> data and the userspace program prints it out.  While this
>> is certainly the recommended approach for most cases, I
>> believe having an in-kernel mechanism would be valuable
>> also.
>>
>> The function the printk() family of functions rely on
>> could potentially be used directly for other use cases
>> like ftrace where we might have the BTF ids of the
>> pointers we wish to display; its signature is as follows:
>>
>> int btf_type_snprintf_show(const struct btf *btf, u32 type_id, void *obj,
>>                             char *buf, int len, u64 flags);
>>
>> So if ftrace say had the BTF ids of the types of arguments,
>> we see that the above would allow us to convert the
>> pointer data into displayable form.
>>
>> To give a flavour for what the printed-out data looks like,
>> here we use pr_info() to display a struct sk_buff *.
>>
>>    struct sk_buff *skb = alloc_skb(64, GFP_KERNEL);
>>
>>    pr_info("%pT", BTF_PTR_TYPE(skb, "struct sk_buff"));
>>
>> ...gives us:
>>
>> (struct sk_buff){
>>   .transport_header = (__u16)65535,
>>   .mac_header = (__u16)65535,
>>   .end = (sk_buff_data_t)192,
>>   .head = (unsigned char *)000000007524fd8b,
>>   .data = (unsigned char *)000000007524fd8b,
> 
> could you add "0x" prefix here to make it even more C like
> and unambiguous ?
> 
>>   .truesize = (unsigned int)768,
>>   .users = (refcount_t){
>>    .refs = (atomic_t){
>>     .counter = (int)1,
>>    },
>>   },
>> }
>>
>> For bpf_trace_printk() a "struct __btf_ptr *" is used as
>> argument; see tools/testing/selftests/bpf/progs/netif_receive_skb.c
>> for example usage.
>>
>> The hope is that this functionality will be useful for debugging,
>> and possibly help facilitate the cases mentioned above in the future.
>>
>> Changes since v1:
>>
>> - changed format to be more drgn-like, rendering indented type info
>>    along with type names by default (Alexei)
>> - zeroed values are omitted (Arnaldo) by default unless the '0'
>>    modifier is specified (Alexei)
>> - added an option to print pointer values without obfuscation.
>>    The reason to do this is the sysctls controlling pointer display
>>    are likely to be irrelevant in many if not most tracing contexts.
>>    Some questions on this in the outstanding questions section below...
>> - reworked printk format specifer so that we no longer rely on format
>>    %pT<type> but instead use a struct * which contains type information
>>    (Rasmus). This simplifies the printk parsing, makes use more dynamic
>>    and also allows specification by BTF id as well as name.
>> - ensured that BTF-specific printk code is bracketed by
>>    #if ENABLED(CONFIG_BTF_PRINTF)
> 
> I don't like this particular bit:
> +config BTF_PRINTF
> +       bool "print type information using BPF type format"
> +       depends on DEBUG_INFO_BTF
> +       default n
> 
> Looks like it's only purpose is to gate printk test.
> In such case please convert it into hidden config that is
> automatically selected when DEBUG_INFO_BTF is set.
> Or just make printk tests to #if IS_ENABLED(DEBUG_INFO_BTF)
> 
>> - removed incorrect patch which tried to fix dereferencing of resolved
>>    BTF info for vmlinux; instead we skip modifiers for the relevant
>>    case (array element type/size determination) (Alexei).
>> - fixed issues with negative snprintf format length (Rasmus)
>> - added test cases for various data structure formats; base types,
>>    typedefs, structs, etc.
>> - tests now iterate through all typedef, enum, struct and unions
>>    defined for vmlinux BTF and render a version of the target dummy
>>    value which is either all zeros or all 0xff values; the idea is this
>>    exercises the "skip if zero" and "print everything" cases.
>> - added support in BPF for using the %pT format specifier in
>>    bpf_trace_printk()
>> - added BPF tests which ensure %pT format specifier use works (Alexei).
>>
>> Outstanding issues
>>
>> - currently %pT is not allowed in BPF programs when lockdown is active
>>    prohibiting BPF_READ; is that sufficient?
> 
> yes. that's enough.
> 
>> - do we want to further restrict the non-obfuscated pointer format
>>    specifier %pTx; for example blocking unprivileged BPF programs from
>>    using it?
> 
> unpriv cannot use bpf_trace_printk() anyway. I'm not sure what is the concern.
> 
>> - likely still need a better answer for vmlinux BTF initialization
>>    than the current approach taken; early boot initialization is one
>>    way to go here.
> 
> btf_parse_vmlinux() can certainly be moved to late_initcall().
> 
>> - may be useful to have a "print integers as hex" format modifier (Joe)
> 
> I'd rather stick to the convention that the type drives the way the value is printed.
> For example:
>    u8 ch = 65;
>    pr_info("%pT", BTF_PTR_TYPE(&ch, "char"));
>    prints 'A'
> while
>    u8 ch = 65;
>    pr_info("%pT", BTF_PTR_TYPE(&ch, "u8"));
>    prints 65
> 
>>
>> Important note: if running test_printf.ko - the version in the bpf-next
>> tree will induce a panic when running the fwnode_pointer() tests due
>> to a kobject issue; applying the patch in
>>
>> https://lkml.org/lkml/2020/4/17/389
> 
> Thanks for the headsup!
> 
> BTF_PTR_ID() is a great idea as well.
> In the llvm 11 we will introduce __builtin__btf_type_id().
> The bpf program will be able to say:
> bpf_printk("%pT", BTF_PTR_ID(skb, __builtin_btf_type_id(skb, 1)));
> That will help avoid run-time search through btf types by string name.
> The compiler will supply btf_id at build time and libbpf will do
> a relocation into vmlinux btf_id prior to loading.

Hi, Just for your information, __builtin_btf_type_id() helper has been
merged in llvm trunk.

https://github.com/llvm/llvm-project/commit/072cde03aaa13a2c57acf62d79876bf79aa1919f

For the above case, you can do
   bpf_printk("%pT", BTF_PTR_ID(skb, __builtin_btf_type_id(*skb, 1)));

Basically,
   __builtin_btf_type_id(*skb, 1)
is to
    - return the type of "*skb", i.e., struct sk_buff
    - generate a relocation (against vmlinux) for this type id

The libbpf work is needed, not implemented yet, for the relocation
to actually happen.

__builtin_btf_type_id(skb, 1) will return a type for "skb" which
is a pointer type to "struct sk_buff".

> 
> Also I think there should be another flag BTF_SHOW_PROBE_DATA.
> It should probe_kernel_read() all data instead of direct dereferences.
> It could be implemented via single probe_kernel_read of the whole BTF data
> structure before pringing the fields one by one. So it should be simple
> addition to patch 2.
> This flag should be default for printk() from bpf program,
> since the verifier won't be checking that pointer passed into bpf_trace_printk
> is of correct btf type and it's a real pointer.
> Whether this flag should be default for normal printk() is arguable.
> I would make it so. The performance cost of extra copy is small comparing
> with no-crash guarantee it provides. I think it would be a good trade off.
> 
> In the future we can extend the verifier so that:
> bpf_safe_printk("%pT", skb);
> will be recognized that 'skb' is PTR_TO_BTF_ID and corresponding and correct
> btf_id is passed into printk. Mistakes will be caught at program
> verification time.
> 

Re: [PATCH v2 bpf-next 6/7] bpf: add support for %pT format specifier for bpf_trace_printk() helper

[Alan Maguire]

On Wed, 13 May 2020, Yonghong Song wrote:

> 
> > +				while (isbtffmt(fmt[i]))
> > +					i++;
> 
> The pointer passed to the helper may not be valid pointer. I think you
> need to do a probe_read_kernel() here. Do an atomic memory allocation
> here should be okay as this is mostly for debugging only.
> 

Are there other examples of doing allocations in program execution
context? I'd hate to be the first to introduce one if not. I was hoping
I could get away with some per-CPU scratch space. Most data structures
will fit within a small per-CPU buffer, but if multiple copies
are required, performance isn't the key concern. It will make traversing
the buffer during display a bit more complex but I think avoiding 
allocation might make that complexity worth it. The other thought I had 
was we could carry out an allocation associated with the attach, 
but that's messy as it's possible run-time might determine the type for
display (and thus the amount of the buffer we need to copy safely).

Great news about LLVM support for __builtin_btf_type_id()!

Thanks!

Alan

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

[Alan Maguire]

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

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.

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

Yeah, I couldn't find a good length to use here.  We
eliminate qualifiers such as "const" in the display, so
it's unlikely we'd overrun.

> > +	} 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.
>

Good idea; will do.
 
> > +	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"?
>

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

> > +	/* 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 = "";

This is wrong for the example case you gave, as we don't want to 
eliminate the opening array parentheses for an array of pointers.

> > +	/* 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.
>

Absolutely.
 
> > +			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?
>

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.

Thanks again for reviewing!

Alan 

Re: [PATCH v2 bpf-next 6/7] bpf: add support for %pT format specifier for bpf_trace_printk() helper

[Yonghong Song]

On 5/18/20 2:10 AM, Alan Maguire wrote:
> On Wed, 13 May 2020, Yonghong Song wrote:
> 
>>
>>> +				while (isbtffmt(fmt[i]))
>>> +					i++;
>>
>> The pointer passed to the helper may not be valid pointer. I think you
>> need to do a probe_read_kernel() here. Do an atomic memory allocation
>> here should be okay as this is mostly for debugging only.
>>
> 
> Are there other examples of doing allocations in program execution
> context? I'd hate to be the first to introduce one if not. I was hoping
> I could get away with some per-CPU scratch space. Most data structures
> will fit within a small per-CPU buffer, but if multiple copies
> are required, performance isn't the key concern. It will make traversing
> the buffer during display a bit more complex but I think avoiding
> allocation might make that complexity worth it. The other thought I had
> was we could carry out an allocation associated with the attach,
> but that's messy as it's possible run-time might determine the type for
> display (and thus the amount of the buffer we need to copy safely).

percpu buffer definitely better. In fact, I am using percpu buffer
in bpf_seq_printf() helper. Yes, you will need to handling contention
though. I guess we can do the same thing here, return -EBUSY so bpf
program can react properly (retry, or just print error, etc.)
if there is a contention.

> 
> Great news about LLVM support for __builtin_btf_type_id()!

Thanks. Hopefully this will make implementation easier.

> 
> Thanks!
> 
> Alan
> 

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
>