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

[RFC PATCH bpf-next 0/6] 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 RFC patchset proposes introducing a "print typed pointer" format
specifier "%pT<type>"; the type specified is then looked up in the BPF
Type Format (BTF) information provided for vmlinux to support display.

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 *.  Note
we specify the 'N' modifier to show type field names:

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

  pr_info("%pTN<struct sk_buff>", skb);

...gives us:

{{{.next=00000000c7916e9c,.prev=00000000c7916e9c,{.dev=00000000c7916e9c|.dev_scratch=0}}|.rbnode={.__rb_parent_color=0,.rb_right=00000000c7916e9c,.rb_left=00000000c7916e9c}|.list={.next=00000000c7916e9c,.prev=00000000c7916e9c}},{.sk=00000000c7916e9c|.ip_defrag_offset=0},{.tstamp=0|.skb_mstamp_ns=0},.cb=['\0'],{{._skb_refdst=0,.destructor=00000000c7916e9c}|.tcp_tsorted_anchor={.next=00000000c7916e9c,.prev=00000000c7916e9c}},._nfct=0,.len=0,.data_len=0,.mac_len=0,.hdr_len=0,.queue_mapping=0,.__cloned_offset=[],.cloned=0x0,.nohdr=0x0,.fclone=0x0,.peeked=0x0,.head_frag=0x0,.pfmemalloc=0x0,.active_extensions=0,.headers_start=[],.__pkt_type_offset=[],.pkt_type=0x0,.ignore_df=0x0,.nf_trace=0x0,.ip_summed=0x0,.ooo_okay=0x0,.l4_hash=0x0,.sw_hash=0x0,.wifi_acked_valid=0x0,.wifi_acked=0x0,.no_fcs=0x0,.encapsulation=0x0,.encap_hdr_csum=0x0,.csum_valid=0x0,.__pkt_vlan_present_offset=[],.vlan_present=0x0,.csum_complete_sw=0x0,.csum_level=0x0,.csum_not_inet=0x0,.dst_pending_co

printk output is truncated at 1024 bytes.  For such cases, the compact
display mode (minus the field info 'N') may be used at the expense of
readability. "|" differentiates between different union members, but
aside from that the format is intended to minic a valid C initiailizer
for the given type.

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

The patches are marked RFC for several reasons

- There's already an RFC patchset in flight dealing with BTF dumping;

https://www.spinics.net/lists/netdev/msg644412.html

  The reason I'm posting this is the approach is a bit different 
  and there may be ways of synthesizing the approaches.
- The mechanism of vmlinux BTF initialization is not fit for purpose
  in a printk() setting as I understand it (it uses mutex locking
  to prevent multiple initializations of the BTF info).  A simple
  approach to support printk might be to simply initialize the
  BTF vmlinux case early in boot; it only needs to happen once.
  Any suggestions here would be great.
- BTF-based rendering is more complex than other printk() format
  specifier-driven methods; that said, because of its generality it
  does provide significant value I think
- More tests are needed.

Alan Maguire (6):
  bpf: provide function to get vmlinux BTF information
  bpf: btf->resolved_[ids,sizes] should not be used for vmlinux BTF
  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<type> format specifier which uses BTF
  printk: extend test_printf to test %pT BTF-based format specifier

 Documentation/core-api/printk-formats.rst |   8 +
 include/linux/bpf.h                       |   2 +
 include/linux/btf.h                       |  35 ++-
 kernel/bpf/btf.c                          | 466 ++++++++++++++++++++++++------
 kernel/bpf/verifier.c                     |  18 +-
 lib/Kconfig                               |  16 +
 lib/test_printf.c                         | 118 ++++++++
 lib/vsprintf.c                            | 145 +++++++++-
 scripts/checkpatch.pl                     |   2 +-
 9 files changed, 704 insertions(+), 106 deletions(-)

-- 
1.8.3.1

[RFC PATCH bpf-next 1/6] 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 40c5392..47d0fcd 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1123,6 +1123,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 ae32517..87251fa 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -10039,6 +10039,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)
 {
@@ -10072,12 +10083,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

[RFC PATCH bpf-next 2/6] bpf: btf->resolved_[ids,sizes] should not be used for vmlinux BTF

[Alan Maguire]

When testing support for print data structures using patches
later in this series, I hit NULL pointer dereference bugs when
printing "struct sk_buff". The problem seems to revolve around
that structure's use of a zero-length array in the middle of
the data structure - headers_start[0].

We see in btf_type_id_size() we consult btf->resolved_ids and
btf->resolved_sizes; both of which are not used in kernel
vmlinux BTF so should not be used when handling vmlinux
BTF data.

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 kernel/bpf/btf.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 50080ad..a474839 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -1170,7 +1170,7 @@ const struct btf_type *btf_type_id_size(const struct btf *btf,
 
 	if (btf_type_has_size(size_type)) {
 		size = size_type->size;
-	} else if (btf_type_is_array(size_type)) {
+	} else if (btf_type_is_array(size_type) && btf->resolved_sizes) {
 		size = btf->resolved_sizes[size_type_id];
 	} else if (btf_type_is_ptr(size_type)) {
 		size = sizeof(void *);
@@ -1179,6 +1179,9 @@ const struct btf_type *btf_type_id_size(const struct btf *btf,
 				 !btf_type_is_var(size_type)))
 			return NULL;
 
+		if (!btf->resolved_ids)
+			return NULL;
+
 		size_type_id = btf->resolved_ids[size_type_id];
 		size_type = btf_type_by_id(btf, size_type_id);
 		if (btf_type_nosize_or_null(size_type))
-- 
1.8.3.1

[RFC PATCH bpf-next 3/6] 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).

Also support flag to emit field information BTF_SHOW_NAME.

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 include/linux/btf.h |  22 +++
 kernel/bpf/btf.c    | 426 ++++++++++++++++++++++++++++++++++++++++------------
 2 files changed, 353 insertions(+), 95 deletions(-)

diff --git a/include/linux/btf.h b/include/linux/btf.h
index 5c1ea99..2f78dc8 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,29 @@ 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);
+
+#define	BTF_SHOW_NAME	(1ULL << 0)
+
 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
+ *	- BTF_SHOW_NAME: show struct/union member names as well as
+ *	  values
+ * 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 a474839..ae453f0 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -281,6 +281,25 @@ 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, ...);
+	/* below are used during iteration */
+	u8 parent_kind;
+	u16 array_encoding;
+	u8 array_terminated;
+	const struct btf *btf;
+	const struct btf_type *type;
+	const struct btf_member *member;
+	char name[KSYM_NAME_LEN];	/* scratch space for member name */
+};
+
 struct btf_kind_operations {
 	s32 (*check_meta)(struct btf_verifier_env *env,
 			  const struct btf_type *t,
@@ -297,14 +316,90 @@ 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];
 static struct btf_type btf_void;
 
+#define btf_show(show, ...)    show->showfn(show, __VA_ARGS__)
+
+static inline void btf_show_start_type(struct btf_show *show,
+				       const struct btf_type *t)
+{
+	show->type = t;
+	show->name[0] = '\0';
+}
+
+static inline void btf_show_end_type(struct btf_show *show,
+				     const char *suffix)
+{
+	if (suffix)
+		btf_show(show, "%s", suffix);
+}
+
+static inline void btf_show_start_array_type(struct btf_show *show,
+					     const struct btf_type *t,
+					     u16 array_encoding)
+{
+	show->array_encoding = array_encoding;
+	show->array_terminated = 0;
+}
+
+static inline void btf_show_end_array_type(struct btf_show *show,
+					   const char *suffix)
+{
+	show->array_encoding = 0;
+	show->array_terminated = 0;
+	btf_show_end_type(show, suffix);
+}
+
+static inline void btf_show_start_member(struct btf_show *show,
+					 const struct btf_member *m)
+{
+	show->member = m;
+}
+
+static inline void btf_show_start_array_member(struct btf_show *show)
+{
+	show->parent_kind = BTF_KIND_ARRAY;
+	btf_show_start_member(show, NULL);
+}
+
+static inline void btf_show_end_member(struct btf_show *show)
+{
+	show->member = NULL;
+	show->parent_kind = BTF_KIND_UNKN;
+}
+
+static inline const char *btf_show_name(struct btf_show *show)
+{
+	const char *member = NULL;
+
+	/*
+	 * Avoid showing member information if we don't have it,
+	 * don't want it or if we're an array member.
+	 */
+	if (!show->member || !(show->flags & BTF_SHOW_NAME) ||
+	    show->parent_kind == BTF_KIND_ARRAY)
+		return "";
+
+	member = btf_name_by_offset(show->btf, show->member->name_off);
+	if (member && strlen(member) > 0)
+		snprintf(show->name, sizeof(show->name), ".%s=",
+			 member);
+
+	return show->name;
+}
+
+#define btf_show_type(show, fmt)				\
+	btf_show(show, "%s" fmt, btf_show_name(show))
+
+#define btf_show_type_value(show, fmt, ...)			\
+	btf_show(show, "%s" fmt, btf_show_name(show), __VA_ARGS__)
+
 static int btf_resolve(struct btf_verifier_env *env,
 		       const struct btf_type *t, u32 type_id);
 
@@ -1252,11 +1347,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,
@@ -1429,7 +1524,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
@@ -1448,9 +1543,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_value(show, "0x%llx%016llx", upper_num,
+				    lower_num);
 }
 
 static void btf_int128_shift(u64 *print_num, u16 left_shift_bits,
@@ -1494,8 +1590,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;
@@ -1515,14 +1611,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);
@@ -1535,55 +1631,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);
+
 	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);
+		btf_int_bits_show(btf, t, data, bits_offset, show);
 		return;
 	}
 
 	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->array_encoding == BTF_INT_CHAR) {
+			/* check for null terminator */
+			if (show->array_terminated)
+				break;
+			if (*(char *)data == '\0') {
+				btf_show_type(show, "'\\0'");
+				show->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);
 	}
+
+	btf_show_end_type(show, NULL);
 }
 
 static const struct btf_kind_operations int_ops = {
@@ -1592,7 +1707,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,
@@ -1856,34 +1971,36 @@ 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)
 {
+	btf_show_start_type(show, t);
 	/* It is a hashed value */
-	seq_printf(m, "%p", *(void **)data);
+	btf_show_type_value(show, "%p", *(void **)data);
+	btf_show_end_type(show, NULL);
 }
 
 static void btf_ref_type_log(struct btf_verifier_env *env,
@@ -1898,7 +2015,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 = {
@@ -1907,7 +2024,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,
@@ -1948,7 +2065,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,
@@ -2107,28 +2224,57 @@ 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;
+	u16 encoding = 0;
 
 	elem_type_id = array->type;
 	elem_type = btf_type_id_size(btf, &elem_type_id, &elem_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, encoding);
+	btf_show_type(show, "[");
+
+	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, ",");
+			btf_show(show, ",");
+
+		btf_show_start_array_member(show);
 
-		elem_ops->seq_show(btf, elem_type, elem_type_id, data,
-				   bits_offset, m);
+		elem_ops->show(btf, elem_type, elem_type_id, data,
+			       bits_offset, show);
 		data += elem_size;
+
+		btf_show_end_member(show);
+
+		if (show->array_terminated)
+			break;
 	}
-	seq_puts(m, "]");
+out:
+	btf_show_end_array_type(show, "]");
 }
 
 static struct btf_kind_operations array_ops = {
@@ -2137,7 +2283,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,
@@ -2360,15 +2506,17 @@ 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_type(show, t);
+	btf_show_type(show, "{");
+
 	for_each_member(i, t, member) {
 		const struct btf_type *member_type = btf_type_by_id(btf,
 								member->type);
@@ -2378,22 +2526,27 @@ static void btf_struct_seq_show(const struct btf *btf, const struct btf_type *t,
 		u8 bits8_offset;
 
 		if (i)
-			seq_puts(m, seq);
+			btf_show(show, 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_type(show, "}");
 }
 
 static struct btf_kind_operations struct_ops = {
@@ -2402,7 +2555,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,
@@ -2533,24 +2686,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);
+
 	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, NULL);
+		return;
 	}
 
-	seq_printf(m, "%d", v);
+	btf_show_type_value(show, "%d", v);
+	btf_show_end_type(show, NULL);
 }
 
 static struct btf_kind_operations enum_ops = {
@@ -2559,7 +2718,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,
@@ -2646,7 +2805,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,
@@ -2680,7 +2839,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,
@@ -2744,7 +2903,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,
@@ -2870,24 +3029,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);
+	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 = {
@@ -2896,7 +3057,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,
@@ -4487,12 +4648,87 @@ 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;
+	show->type = NULL;
+	show->member = NULL;
+
+	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 = 0;
+
+	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;
+
+	if (ssnprintf->len < 0)
+		return;
+
+	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

[RFC PATCH bpf-next 4/6] checkpatch: add new BTF pointer format specifier

[Alan Maguire]

checkoatch 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 a63380c..1683a26 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -6028,7 +6028,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

[RFC PATCH bpf-next 5/6] printk: add type-printing %pT<type> 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 a suffix
enclosed <like this> specifies the type, for example, specifying

	printk("%pT<struct sk_buff>", skb)

...will utilize BTF information to traverse the struct 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 compact, specifying values only, but the
'N' modifier can be used to show field names to more easily
track values.  Pointer values are obfuscated as usual.  As
an example:

  struct sk_buff *skb = alloc_skb(64, GFP_KERNEL);
  pr_info("%pTN<struct sk_buff>", skb);

...gives us:

{{{.next=00000000c7916e9c,.prev=00000000c7916e9c,{.dev=00000000c7916e9c|.dev_scratch=0}}|.rbnode={.__rb_parent_color=0,.rb_right=00000000c7916e9c,.rb_left=00000000c7916e9c}|.list={.next=00000000c7916e9c,.prev=00000000c7916e9c}},{.sk=00000000c7916e9c|.ip_defrag_offset=0},{.tstamp=0|.skb_mstamp_ns=0},.cb=['\0'],{{._skb_refdst=0,.destructor=00000000c7916e9c}|.tcp_tsorted_anchor={.next=00000000c7916e9c,.prev=00000000c7916e9c}},._nfct=0,.len=0,.data_len=0,.mac_len=0,.hdr_len=0,.queue_mapping=0,.__cloned_offset=[],.cloned=0x0,.nohdr=0x0,.fclone=0x0,.peeked=0x0,.head_frag=0x0,.pfmemalloc=0x0,.active_extensions=0,.headers_start=[],.__pkt_type_offset=[],.pkt_type=0x0,.ignore_df=0x0,.nf_trace=0x0,.ip_summed=0x0,.ooo_okay=0x0,.l4_hash=0x0,.sw_hash=0x0,.wifi_acked_valid=0x0,.wifi_acked=0x0,.no_fcs=0x0,.encapsulation=0x0,.encap_hdr_csum=0x0,.csum_valid=0x0,.__pkt_vlan_present_offset=[],.vlan_present=0x0,.csum_complete_sw=0x0,.csum_level=0x0,.csum_not_inet=0x0,.dst_pending_co

printk output is truncated at 1024 bytes.  For such cases, the compact
display mode (minus the field info) may be used. "|" differentiates
between different union members.

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 Documentation/core-api/printk-formats.rst |   8 ++
 include/linux/btf.h                       |   3 +-
 lib/Kconfig                               |  16 ++++
 lib/vsprintf.c                            | 145 +++++++++++++++++++++++++++++-
 4 files changed, 169 insertions(+), 3 deletions(-)

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index 8ebe46b1..b786577 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -545,6 +545,14 @@ For printing netdev_features_t.
 
 Passed by reference.
 
+BTF-based printing of pointer data
+----------------------------------
+If '%pT[N]<type_name>' is specified, use the BPF Type Format (BTF) to
+show the typed data.  For example, specifying '%pT<struct sk_buff>' will utilize
+BTF information to traverse the struct sk_buff * and display it.
+
+Supported modifer is 'N' (show type field names).
+
 Thanks
 ======
 
diff --git a/include/linux/btf.h b/include/linux/btf.h
index 2f78dc8..456bd8f 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -158,10 +158,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/lib/Kconfig b/lib/Kconfig
index bc7e563..e92109e 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..43e06f3 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,127 @@ char *fwnode_string(char *buf, char *end, struct fwnode_handle *fwnode,
 	return widen_string(buf, buf - buf_start, end, spec);
 }
 
+#define is_btf_fmt_start(c)	(c == 'T')
+#define is_btf_type_start(c)	(c == '<')
+#define is_btf_type_end(c)	(c == '>')
+
+#define btf_modifier_flag(c)	(c == 'N' ? BTF_SHOW_NAME : 0)
+
+static noinline_for_stack
+const char *skip_btf_type(const char *fmt, bool *found_btf_type)
+{
+	*found_btf_type = false;
+
+	if (!is_btf_fmt_start(*fmt))
+		return fmt;
+	fmt++;
+
+	while (btf_modifier_flag(*fmt))
+		fmt++;
+
+	if (!is_btf_type_start(*fmt))
+		return fmt;
+
+	while (!is_btf_type_end(*fmt) && *fmt != '\0')
+		fmt++;
+
+	if (is_btf_type_end(*fmt)) {
+		fmt++;
+		*found_btf_type = true;
+	}
+
+	return fmt;
+}
+
+static noinline_for_stack
+char *btf_string(char *buf, char *end, void *ptr, struct printf_spec spec,
+		 const char *fmt)
+{
+	const struct btf_type *btf_type;
+	char btf_name[KSYM_SYMBOL_LEN];
+	u8 btf_kind = BTF_KIND_TYPEDEF;
+	const struct btf *btf;
+	char *buf_start = buf;
+	u64 flags = 0, mod;
+	s32 btf_id;
+	int i;
+
+	/*
+	 * Accepted format is [format_modifiers]*<type> ;
+	 * for example "%pTN<struct sk_buff>" will show a representation
+	 * of the sk_buff pointed to by the associated argument including
+	 * member names.
+	 */
+	if (check_pointer(&buf, end, ptr, spec))
+		return buf;
+
+	while (isalpha(*fmt)) {
+		mod = btf_modifier_flag(*fmt);
+		if (!mod)
+			break;
+		flags |= mod;
+		fmt++;
+	}
+
+	if (!is_btf_type_start(*fmt))
+		return error_string(buf, end, "(%pT?)", spec);
+	fmt++;
+
+	if (isspace(*fmt))
+		fmt = skip_spaces(++fmt);
+
+	if (strncmp(fmt, "struct ", strlen("struct ")) == 0) {
+		btf_kind = BTF_KIND_STRUCT;
+		fmt += strlen("struct ");
+	} else if (strncmp(fmt, "union ", strlen("union ")) == 0) {
+		btf_kind = BTF_KIND_UNION;
+		fmt += strlen("union ");
+	} else if (strncmp(fmt, "enum ", strlen("enum ")) == 0) {
+		btf_kind = BTF_KIND_ENUM;
+		fmt += strlen("enum ");
+	}
+
+	if (isspace(*fmt))
+		fmt = skip_spaces(++fmt);
+
+	for (i = 0; isalnum(*fmt) || *fmt == '_'; fmt++, i++)
+		btf_name[i] = *fmt;
+
+	btf_name[i] = '\0';
+
+	if (isspace(*fmt))
+		fmt = skip_spaces(++fmt);
+
+	if (strlen(btf_name) == 0 || !is_btf_type_end(*fmt))
+		return error_string(buf, end, "(%pT?)", spec);
+
+	btf = bpf_get_btf_vmlinux();
+	if (IS_ERR_OR_NULL(btf))
+		return ptr_to_id(buf, end, ptr, spec);
+
+	/*
+	 * Assume type specified is a typedef as there's not much
+	 * benefit in specifying %p<int> 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_name, btf_kind);
+	if (btf_id < 0)
+		btf_id = btf_find_by_name_kind(btf, btf_name,
+					       BTF_KIND_INT);
+
+	if (btf_id >= 0)
+		btf_type = btf_type_by_id(btf, btf_id);
+	if (btf_id < 0 || !btf_type)
+		return ptr_to_id(buf, end, ptr, spec);
+
+	buf += btf_type_snprintf_show(btf, btf_id, ptr, buf,
+				      end - buf_start, flags);
+
+	return widen_string(buf, buf - buf_start, end, spec);
+}
+
 /*
  * 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 +2291,15 @@ 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[N<type_name>]' For printing pointer data using BPF Type Format (BTF).
+ *
+ *			Optional arguments are
+ *			N		print type and member names
+ *
+ *			Required options are
+ *			<type_name>	associated pointer is interpreted
+ *					to point at type_name.
+ *
  * ** When making changes please also update:
  *	Documentation/core-api/printk-formats.rst
  *
@@ -2251,6 +2382,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 */
@@ -2506,6 +2639,7 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 	unsigned long long num;
 	char *str, *end;
 	struct printf_spec spec = {0};
+	bool found_btf_type;
 
 	/* Reject out-of-range values early.  Large positive sizes are
 	   used for unknown buffer sizes. */
@@ -2577,8 +2711,15 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 		case FORMAT_TYPE_PTR:
 			str = pointer(fmt, str, end, va_arg(args, void *),
 				      spec);
-			while (isalnum(*fmt))
-				fmt++;
+			/*
+			 * BTF type info is enclosed <like this>, so can
+			 * contain whitespace.
+			 */
+			fmt = skip_btf_type(fmt, &found_btf_type);
+			if (!found_btf_type) {
+				while (isalnum(*fmt))
+					fmt++;
+			}
 			break;
 
 		case FORMAT_TYPE_PERCENT_CHAR:
-- 
1.8.3.1

[RFC PATCH bpf-next 6/6] printk: extend test_printf to test %pT BTF-based format specifier

[Alan Maguire]

Add tests to verify basic types 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 10,000 new tests covering
all enum, struct, union and typedefs in vmlinux BTF.

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

diff --git a/include/linux/btf.h b/include/linux/btf.h
index 456bd8f..ef66d2e 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -177,4 +177,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 ae453f0..0703d1d 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -4867,3 +4867,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..9743e96 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,120 @@ static void __init fwnode_pointer(void)
 #endif
 }
 
+#define	__TEST_BTF(type, var, expected)	test(expected, "%pT<"#type">", &var)
+
+#define TEST_BTF(type, var, ...)					\
+	do {								\
+		type var = __VA_ARGS__;					\
+		pr_debug("type %s: %pT<" #type ">", #type, &var);	\
+		__TEST_BTF(type, var, #__VA_ARGS__);			\
+	} while (0)
+
+#define	BTF_MAX_DATA_SIZE	8192
+#define	BTF_MAX_BUF_SIZE	(BTF_MAX_DATA_SIZE * 8)
+
+static void __init
+btf_print_kind(u8 kind, const char *kind_name)
+{
+	char fmt1[256], fmt2[256];
+	int res1, res2, res3;
+	const char *name;
+	u64 *dummy_data;
+	s32 id = 0;
+	char *buf;
+
+	dummy_data = kzalloc(BTF_MAX_DATA_SIZE, GFP_KERNEL);
+	buf = kzalloc(BTF_MAX_BUF_SIZE, GFP_KERNEL);
+	for (;;) {
+		name = btf_vmlinux_next_type_name(kind, &id);
+		if (!name)
+			break;
+
+		total_tests++;
+
+		strncpy(fmt1, "%pT<", sizeof(fmt1));
+		strncat(fmt1, kind_name, sizeof(fmt1));
+		strncat(fmt1, name, sizeof(fmt1));
+		strncat(fmt1, ">", sizeof(fmt1));
+
+		strncpy(fmt2, "%pTN<", sizeof(fmt2));
+		strncat(fmt2, kind_name, sizeof(fmt2));
+		strncat(fmt2, name, sizeof(fmt2));
+		strncat(fmt2, ">", sizeof(fmt2));
+
+		res1 = snprintf(buf, BTF_MAX_BUF_SIZE, fmt1, dummy_data);
+		res2 = snprintf(buf, 0, fmt1, dummy_data);
+		res3 = snprintf(buf, BTF_MAX_BUF_SIZE, fmt2, dummy_data);
+
+		/*
+		 * 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) {
+			pr_warn("snprintf(%s%s); %d <= 0",
+				kind_name, name,
+				res1 <= 0 ? res1 : res2 <= 0 ? res2 : res3);
+			failed_tests++;
+		} else if (res1 != res2) {
+			pr_warn("snprintf(%s%s): %d != %d",
+				kind_name, name, res1, res2);
+			failed_tests++;
+		} else if (res3 < res2) {
+			pr_warn("snprintf(%s%s); %d < %d",
+				kind_name, name, res3, res2);
+			failed_tests++;
+		} else {
+			pr_debug("Printed %s%s (%d bytes, %d bytes with names)",
+				 kind_name, name, res1, res3);
+		}
+	}
+	kfree(dummy_data);
+	kfree(buf);
+}
+
+static void __init
+btf_pointer(void)
+{
+	struct sk_buff *skb = alloc_skb(64, GFP_KERNEL);
+#ifdef CONFIG_BTF_PRINTF
+	TEST_BTF(int, testint, 0);
+	TEST_BTF(int, testint, 1234);
+	TEST_BTF(int, testint, -4567);
+	TEST_BTF(bool, testbool, 0);
+	TEST_BTF(bool, testbool, 1);
+	TEST_BTF(int64_t, testint64, 0);
+	TEST_BTF(int64_t, testint64, 1234);
+	TEST_BTF(int64_t, testint64, -4567);
+	TEST_BTF(char, testchar, 100);
+	TEST_BTF(enum bpf_arg_type, testenum, ARG_CONST_MAP_PTR);
+#endif /* CONFIG_BTF_PRINTF */
+
+	/*
+	 * Iterate every instance of each kind, printing each associated type.
+	 * This constitutes around 10k tests.
+	 */
+	btf_print_kind(BTF_KIND_STRUCT, "struct ");
+	btf_print_kind(BTF_KIND_UNION, "union ");
+	btf_print_kind(BTF_KIND_ENUM, "enum ");
+	btf_print_kind(BTF_KIND_TYPEDEF, "");
+
+	/* verify unknown type falls back to hashed pointer display */
+	test_hashed("%pT<unknown_type>", NULL);
+	test_hashed("%pT<unknown_type>", skb);
+
+	/* verify use of unknown modifier X returns error string */
+	test("(%pT?)<unknown_type>", "%pTX<unknown_type>", skb);
+
+	/* No space separation is allowed other than for struct|enum|union */
+	test("(%pT?)", "%pT<invalid format>", skb);
+	/* Missing ">" format error */
+	test("(%pT?)", "%pT<struct sk_buff", skb);
+
+	kfree_skb(skb);
+}
+
 static void __init
 test_pointer(void)
 {
@@ -668,6 +785,7 @@ static void __init fwnode_pointer(void)
 	flags();
 	errptr();
 	fwnode_pointer();
+	btf_pointer();
 }
 
 static void __init selftest(void)
-- 
1.8.3.1

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

[Arnaldo Carvalho de Melo]

Em Fri, Apr 17, 2020 at 11:42:34AM +0100, Alan Maguire escreveu:
> To give a flavour for what the printed-out data looks like,
> here we use pr_info() to display a struct sk_buff *.  Note
> we specify the 'N' modifier to show type field names:
> 
>   struct sk_buff *skb = alloc_skb(64, GFP_KERNEL);
> 
>   pr_info("%pTN<struct sk_buff>", skb);
> 
> ...gives us:
> 
> {{{.next=00000000c7916e9c,.prev=00000000c7916e9c,{.dev=00000000c7916e9c|.dev_scratch=0}}|.rbnode={.__rb_parent_color=0,.rb_right=00000000c7916e9c,.rb_left=00000000c7916e9c}|.list={.next=00000000c7916e9c,.prev=00000000c7916e9c}},{.sk=00000000c7916e9c|.ip_defrag_offset=0},{.tstamp=0|.skb_mstamp_ns=0},.cb=['\0'],{{._skb_refdst=0,.destructor=00000000c7916e9c}|.tcp_tsorted_anchor={.next=00000000c7916e9c,.prev=00000000c7916e9c}},._nfct=0,.len=0,.data_len=0,.mac_len=0,.hdr_len=0,.queue_mapping=0,.__cloned_offset=[],.cloned=0x0,.nohdr=0x0,.fclone=0x0,.peeked=0x0,.head_frag=0x0,.pfmemalloc=0x0,.active_extensions=0,.headers_start=[],.__pkt_type_offset=[],.pkt_type=0x0,.ignore_df=0x0,.nf_trace=0x0,.ip_summed=0x0,.ooo_okay=0x0,.l4_hash=0x0,.sw_hash=0x0,.wifi_acked_valid=0x0,.wifi_acked=0x0,.no_fcs=0x0,.encapsulation=0x0,.encap_hdr_csum=0x0,.csum_valid=0x0,.__pkt_vlan_present_offset=[],.vlan_present=0x0,.csum_complete_sw=0x0,.csum_level=0x0,.csum_not_inet=0x0,.dst_pending_co

One suggestion, to make this more compact, one could have %pTNz<struct
sk_buff>" that wouldn't print any integral type member that is zeroed
:-)
 
- Arnaldo

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

[Alan Maguire]

On Fri, 17 Apr 2020, Arnaldo Carvalho de Melo wrote:

> Em Fri, Apr 17, 2020 at 11:42:34AM +0100, Alan Maguire escreveu:
> > To give a flavour for what the printed-out data looks like,
> > here we use pr_info() to display a struct sk_buff *.  Note
> > we specify the 'N' modifier to show type field names:
> > 
> >   struct sk_buff *skb = alloc_skb(64, GFP_KERNEL);
> > 
> >   pr_info("%pTN<struct sk_buff>", skb);
> > 
> > ...gives us:
> > 
> > {{{.next=00000000c7916e9c,.prev=00000000c7916e9c,{.dev=00000000c7916e9c|.dev_scratch=0}}|.rbnode={.__rb_parent_color=0,.rb_right=00000000c7916e9c,.rb_left=00000000c7916e9c}|.list={.next=00000000c7916e9c,.prev=00000000c7916e9c}},{.sk=00000000c7916e9c|.ip_defrag_offset=0},{.tstamp=0|.skb_mstamp_ns=0},.cb=['\0'],{{._skb_refdst=0,.destructor=00000000c7916e9c}|.tcp_tsorted_anchor={.next=00000000c7916e9c,.prev=00000000c7916e9c}},._nfct=0,.len=0,.data_len=0,.mac_len=0,.hdr_len=0,.queue_mapping=0,.__cloned_offset=[],.cloned=0x0,.nohdr=0x0,.fclone=0x0,.peeked=0x0,.head_frag=0x0,.pfmemalloc=0x0,.active_extensions=0,.headers_start=[],.__pkt_type_offset=[],.pkt_type=0x0,.ignore_df=0x0,.nf_trace=0x0,.ip_summed=0x0,.ooo_okay=0x0,.l4_hash=0x0,.sw_hash=0x0,.wifi_acked_valid=0x0,.wifi_acked=0x0,.no_fcs=0x0,.encapsulation=0x0,.encap_hdr_csum=0x0,.csum_valid=0x0,.__pkt_vlan_present_offset=[],.vlan_present=0x0,.csum_complete_sw=0x0,.csum_level=0x0,.csum_not_inet=0x0,.dst_pending_co
> 
> One suggestion, to make this more compact, one could have %pTNz<struct
> sk_buff>" that wouldn't print any integral type member that is zeroed
> :-)
>

That's a great idea, thanks Arnaldo! I'll add that.

Alan
 
> - Arnaldo
> 

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

[Alexei Starovoitov]

On Fri, Apr 17, 2020 at 11:42:34AM +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 RFC patchset proposes introducing a "print typed pointer" format
> specifier "%pT<type>"; the type specified is then looked up in the BPF
> Type Format (BTF) information provided for vmlinux to support display.

This is great idea! Love it.

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

yep. This is useful for general purpose printk.
The only piece that must be highlighted in the printk documentation
that unlike the rest of BPF there are zero safety guarantees here.
The programmer can pass wrong pointer to printk() and the kernel _will_ crash.

>   struct sk_buff *skb = alloc_skb(64, GFP_KERNEL);
> 
>   pr_info("%pTN<struct sk_buff>", skb);

why follow "TN" convention?
I think "%p<struct sk_buff>" is much more obvious, unambiguous, and
equally easy to parse.

> ...gives us:
> 
> {{{.next=00000000c7916e9c,.prev=00000000c7916e9c,{.dev=00000000c7916e9c|.dev_scratch=0}}|.rbnode={.__rb_parent_color=0,

This is unreadable.
I like the choice of C style output, but please format it similar to drgn. Like:
*(struct task_struct *)0xffff889ff8a08000 = {
	.thread_info = (struct thread_info){
		.flags = (unsigned long)0,
		.status = (u32)0,
	},
	.state = (volatile long)1,
	.stack = (void *)0xffffc9000c4dc000,
	.usage = (refcount_t){
		.refs = (atomic_t){
			.counter = (int)2,
		},
	},
	.flags = (unsigned int)4194560,
	.ptrace = (unsigned int)0,

I like Arnaldo's idea as well, but I prefer zeros to be dropped by default.
Just like %d doesn't print leading zeros by default.
"%p0<struct sk_buff>" would print them.

> The patches are marked RFC for several reasons
> 
> - There's already an RFC patchset in flight dealing with BTF dumping;
> 
> https://www.spinics.net/lists/netdev/msg644412.html
> 
>   The reason I'm posting this is the approach is a bit different 
>   and there may be ways of synthesizing the approaches.

I see no overlap between patch sets whatsoever.
Why do you think there is?

> - The mechanism of vmlinux BTF initialization is not fit for purpose
>   in a printk() setting as I understand it (it uses mutex locking
>   to prevent multiple initializations of the BTF info).  A simple
>   approach to support printk might be to simply initialize the
>   BTF vmlinux case early in boot; it only needs to happen once.
>   Any suggestions here would be great.
> - BTF-based rendering is more complex than other printk() format
>   specifier-driven methods; that said, because of its generality it
>   does provide significant value I think
> - More tests are needed.

yep. Please make sure to add one to selftest/bpf as well.
bpf maintainers don't run printk tests as part of workflow, so
future BTF changes will surely break it if there are no selftests/bpf.

Patch 2 isn't quite correct. Early parse of vmlinux BTF does not compute
resolved_ids to save kernel memory. The trade off is execution time vs kernel
memory. I believe that saving memory is more important here, since execution is
not in critical path. There is __get_type_size(). It should be used in later
patches instead of btf_type_id_size() that relies on pre-computed
resolved_sizes and resolved_ids.

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

[Arnaldo Melo]

On April 18, 2020 1:05:36 PM GMT-03:00, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
>On Fri, Apr 17, 2020 at 11:42:34AM +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 RFC patchset proposes introducing a "print typed pointer" format
>> specifier "%pT<type>"; the type specified is then looked up in the
>BPF
>> Type Format (BTF) information provided for vmlinux to support
>display.
>
>This is great idea! Love it.

21st century finally! 8-)

>> 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.
>
>yep. This is useful for general purpose printk.
>The only piece that must be highlighted in the printk documentation
>that unlike the rest of BPF there are zero safety guarantees here.
>The programmer can pass wrong pointer to printk() and the kernel _will_
>crash.
>
>>   struct sk_buff *skb = alloc_skb(64, GFP_KERNEL);
>> 
>>   pr_info("%pTN<struct sk_buff>", skb);
>
>why follow "TN" convention?
>I think "%p<struct sk_buff>" is much more obvious, unambiguous, and
>equally easy to parse.
>
>> ...gives us:
>> 
>>
>{{{.next=00000000c7916e9c,.prev=00000000c7916e9c,{.dev=00000000c7916e9c|.dev_scratch=0}}|.rbnode={.__rb_parent_color=0,
>
>This is unreadable.
>I like the choice of C style output, but please format it similar to
>drgn. Like:
>*(struct task_struct *)0xffff889ff8a08000 = {
>	.thread_info = (struct thread_info){
>		.flags = (unsigned long)0,
>		.status = (u32)0,
>	},
>	.state = (volatile long)1,
>	.stack = (void *)0xffffc9000c4dc000,
>	.usage = (refcount_t){
>		.refs = (atomic_t){
>			.counter = (int)2,
>		},
>	},
>	.flags = (unsigned int)4194560,
>	.ptrace = (unsigned int)0,
>
>I like Arnaldo's idea as well, but I prefer zeros to be dropped by
>default.

That's my preference as well, it's just I feel ashamed of not participating in this party as much as I feel I should, so I was just being overly cautious in my suggestions.

'perf trace'  zaps any zero syscall arg out of the way by default, so that's my preference, sure.

 - Arnaldo

>Just like %d doesn't print leading zeros by default.
>"%p0<struct sk_buff>" would print them.
>
>> The patches are marked RFC for several reasons
>> 
>> - There's already an RFC patchset in flight dealing with BTF dumping;
>> 
>> https://www.spinics.net/lists/netdev/msg644412.html
>> 
>>   The reason I'm posting this is the approach is a bit different 
>>   and there may be ways of synthesizing the approaches.
>
>I see no overlap between patch sets whatsoever.
>Why do you think there is?
>
>> - The mechanism of vmlinux BTF initialization is not fit for purpose
>>   in a printk() setting as I understand it (it uses mutex locking
>>   to prevent multiple initializations of the BTF info).  A simple
>>   approach to support printk might be to simply initialize the
>>   BTF vmlinux case early in boot; it only needs to happen once.
>>   Any suggestions here would be great.
>> - BTF-based rendering is more complex than other printk() format
>>   specifier-driven methods; that said, because of its generality it
>>   does provide significant value I think
>> - More tests are needed.
>
>yep. Please make sure to add one to selftest/bpf as well.
>bpf maintainers don't run printk tests as part of workflow, so
>future BTF changes will surely break it if there are no selftests/bpf.
>
>Patch 2 isn't quite correct. Early parse of vmlinux BTF does not
>compute
>resolved_ids to save kernel memory. The trade off is execution time vs
>kernel
>memory. I believe that saving memory is more important here, since
>execution is
>not in critical path. There is __get_type_size(). It should be used in
>later
>patches instead of btf_type_id_size() that relies on pre-computed
>resolved_sizes and resolved_ids.

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

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

[Alan Maguire]

On Sat, 18 Apr 2020, Alexei Starovoitov wrote:

> On Fri, Apr 17, 2020 at 11:42:34AM +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 RFC patchset proposes introducing a "print typed pointer" format
> > specifier "%pT<type>"; the type specified is then looked up in the BPF
> > Type Format (BTF) information provided for vmlinux to support display.
> 
> This is great idea! Love it.
>

Thanks for taking a look!
 
> > 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.
> 
> yep. This is useful for general purpose printk.
> The only piece that must be highlighted in the printk documentation
> that unlike the rest of BPF there are zero safety guarantees here.
> The programmer can pass wrong pointer to printk() and the kernel _will_ crash.
> 

Good point; I'll highlight the fact that we aren't
executing in BPF context, no verifier etc.

> >   struct sk_buff *skb = alloc_skb(64, GFP_KERNEL);
> > 
> >   pr_info("%pTN<struct sk_buff>", skb);
> 
> why follow "TN" convention?
> I think "%p<struct sk_buff>" is much more obvious, unambiguous, and
> equally easy to parse.
> 

That was my first choice, but the first character
after the 'p' in the '%p' specifier signifies the
pointer format specifier. If we use '<', and have
'%p<', where do we put the modifiers? '%p<xYz struct foo>'
seems clunky to me.

> > ...gives us:
> > 
> > {{{.next=00000000c7916e9c,.prev=00000000c7916e9c,{.dev=00000000c7916e9c|.dev_scratch=0}}|.rbnode={.__rb_parent_color=0,
> 
> This is unreadable.
> I like the choice of C style output, but please format it similar to drgn. Like:
> *(struct task_struct *)0xffff889ff8a08000 = {
> 	.thread_info = (struct thread_info){
> 		.flags = (unsigned long)0,
> 		.status = (u32)0,
> 	},
> 	.state = (volatile long)1,
> 	.stack = (void *)0xffffc9000c4dc000,
> 	.usage = (refcount_t){
> 		.refs = (atomic_t){
> 			.counter = (int)2,
> 		},
> 	},
> 	.flags = (unsigned int)4194560,
> 	.ptrace = (unsigned int)0,
> 
> I like Arnaldo's idea as well, but I prefer zeros to be dropped by default.
> Just like %d doesn't print leading zeros by default.
> "%p0<struct sk_buff>" would print them.
> 

I'll try and match the above as closely as possible for v2
while retaining the compact form for the verifier's use.

> > The patches are marked RFC for several reasons
> > 
> > - There's already an RFC patchset in flight dealing with BTF dumping;
> > 
> > https://www.spinics.net/lists/netdev/msg644412.html
> > 
> >   The reason I'm posting this is the approach is a bit different 
> >   and there may be ways of synthesizing the approaches.
> 
> I see no overlap between patch sets whatsoever.
> Why do you think there is?
>

Because I hadn't read through Yonghong's patchset properly ;-)
I see potential future overlap in having a dumper support a 
"raw" mode using BTF-based display if needed, but no actual
overlap in what's there (and here) today.
 
> > - The mechanism of vmlinux BTF initialization is not fit for purpose
> >   in a printk() setting as I understand it (it uses mutex locking
> >   to prevent multiple initializations of the BTF info).  A simple
> >   approach to support printk might be to simply initialize the
> >   BTF vmlinux case early in boot; it only needs to happen once.
> >   Any suggestions here would be great.
> > - BTF-based rendering is more complex than other printk() format
> >   specifier-driven methods; that said, because of its generality it
> >   does provide significant value I think
> > - More tests are needed.
> 
> yep. Please make sure to add one to selftest/bpf as well.
> bpf maintainers don't run printk tests as part of workflow, so
> future BTF changes will surely break it if there are no selftests/bpf.
> 

Absolutely.

> Patch 2 isn't quite correct. Early parse of vmlinux BTF does not compute
> resolved_ids to save kernel memory. The trade off is execution time vs kernel
> memory. I believe that saving memory is more important here, since execution is
> not in critical path. There is __get_type_size(). It should be used in later
> patches instead of btf_type_id_size() that relies on pre-computed
> resolved_sizes and resolved_ids.
>

Thanks for the info, will fix for v2!

Alan 

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

[Joe Perches]

On Mon, 2020-04-20 at 16:29 +0100, Alan Maguire wrote:
> On Sat, 18 Apr 2020, Alexei Starovoitov wrote:
> 
> > On Fri, Apr 17, 2020 at 11:42:34AM +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 RFC patchset proposes introducing a "print typed pointer" format
> > > specifier "%pT<type>"; the type specified is then looked up in the BPF
> > > Type Format (BTF) information provided for vmlinux to support display.
> > 
> > This is great idea! Love it.
> > 
> 
> Thanks for taking a look!
>  
> > > 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.
> > 
> > yep. This is useful for general purpose printk.
> > The only piece that must be highlighted in the printk documentation
> > that unlike the rest of BPF there are zero safety guarantees here.
> > The programmer can pass wrong pointer to printk() and the kernel _will_ crash.
> > 
> 
> Good point; I'll highlight the fact that we aren't
> executing in BPF context, no verifier etc.
> 
> > >   struct sk_buff *skb = alloc_skb(64, GFP_KERNEL);
> > > 
> > >   pr_info("%pTN<struct sk_buff>", skb);
> > 
> > why follow "TN" convention?
> > I think "%p<struct sk_buff>" is much more obvious, unambiguous, and
> > equally easy to parse.
> > 
> 
> That was my first choice, but the first character
> after the 'p' in the '%p' specifier signifies the
> pointer format specifier. If we use '<', and have
> '%p<', where do we put the modifiers? '%p<xYz struct foo>'
> seems clunky to me.

While I don't really like the %p<struct type> block,
it's at least obvious what's being attempted.

Modifiers could easily go after the <struct type> block.

It appears a %p<struct type> output might be a lot of
total characters so another potential issue might be
the maximum length of each individual printk.

> > I like the choice of C style output, but please format it similar to drgn. Like:
> > *(struct task_struct *)0xffff889ff8a08000 = {
> > 	.thread_info = (struct thread_info){
> > 		.flags = (unsigned long)0,
> > 		.status = (u32)0,
> > 	},
> > 	.state = (volatile long)1,
> > 	.stack = (void *)0xffffc9000c4dc000,
> > 	.usage = (refcount_t){
> > 		.refs = (atomic_t){
> > 			.counter = (int)2,
> > 		},
> > 	},
> > 	.flags = (unsigned int)4194560,
> > 	.ptrace = (unsigned int)0,

And here, the issue might be duplicating the log level
for each line of output and/or prefixing each line with
whatever struct type is being dumped as interleaving
with other concurrent logging is possible.

Here as well the individual field types don't contain
enough information to determine if a field should be
output as %x or %u.

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

[Arnaldo Carvalho de Melo]

Em Sat, Apr 18, 2020 at 09:05:36AM -0700, Alexei Starovoitov escreveu:
> On Fri, Apr 17, 2020 at 11:42:34AM +0100, Alan Maguire wrote:
> > ...gives us:
> > 
> > {{{.next=00000000c7916e9c,.prev=00000000c7916e9c,{.dev=00000000c7916e9c|.dev_scratch=0}}|.rbnode={.__rb_parent_color=0,
 
> This is unreadable.
> I like the choice of C style output, but please format it similar to drgn. Like:
> *(struct task_struct *)0xffff889ff8a08000 = {
> 	.thread_info = (struct thread_info){
> 		.flags = (unsigned long)0,
> 		.status = (u32)0,
> 	},
> 	.state = (volatile long)1,
> 	.stack = (void *)0xffffc9000c4dc000,
> 	.usage = (refcount_t){
> 		.refs = (atomic_t){
> 			.counter = (int)2,
> 		},
> 	},
> 	.flags = (unsigned int)4194560,
> 	.ptrace = (unsigned int)0,
 
> I like Arnaldo's idea as well, but I prefer zeros to be dropped by default.
> Just like %d doesn't print leading zeros by default.
> "%p0<struct sk_buff>" would print them.

I was thinking about another way to compress the output of a given data
structure someone is tracking, having to print it from time to time,
which is to store a copy of the struct as you print it and then, when
printing it again, print just its pointer, i.e. that:

*(struct task_struct *)0xffff889ff8a08000 = {

Line, then just printing the fields that changed, say just that refcount
was bumped, so it first print:

*(struct task_struct *)0xffff889ff8a08000 = {
      .thread_info = (struct thread_info){
              .flags = (unsigned long)0,
              .status = (u32)0,
      },
      .state = (volatile long)1,
      .stack = (void *)0xffffc9000c4dc000,
      .usage = (refcount_t){
              .refs = (atomic_t){
                      .counter = (int)2,
              },
      },
      .flags = (unsigned int)4194560,
      .ptrace = (unsigned int)0,

Then, the next time it would print:

*(struct task_struct *)0xffff889ff8a08000 = {
      .usage = (refcount_t){
              .refs = (atomic_t){
                      .counter = (int)3,
              },
      },
},

- Arnaldo

Re: [RFC PATCH bpf-next 5/6] printk: add type-printing %pT<type> format specifier which uses BTF

[Rasmus Villemoes]

On 17/04/2020 12.42, 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 a suffix
> enclosed <like this> specifies the type, for example, specifying
> 
> 	printk("%pT<struct sk_buff>", skb)
> 
> ...will utilize BTF information to traverse the struct 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 compact, specifying values only, but the
> 'N' modifier can be used to show field names to more easily
> track values.  Pointer values are obfuscated as usual.  As
> an example:
> 
>   struct sk_buff *skb = alloc_skb(64, GFP_KERNEL);
>   pr_info("%pTN<struct sk_buff>", skb);
> 
> ...gives us:
> 
> {{{.next=00000000c7916e9c,.prev=00000000c7916e9c,{.dev=00000000c7916e9c|.dev_scratch=0}}|.rbnode={.__rb_parent_color=0,.rb_right=00000000c7916e9c,.rb_left=00000000c7916e9c}|.list={.next=00000000c7916e9c,.prev=00000000c7916e9c}},{.sk=00000000c7916e9c|.ip_defrag_offset=0},{.tstamp=0|.skb_mstamp_ns=0},.cb=['\0'],{{._skb_refdst=0,.destructor=00000000c7916e9c}|.tcp_tsorted_anchor={.next=00000000c7916e9c,.prev=00000000c7916e9c}},._nfct=0,.len=0,.data_len=0,.mac_len=0,.hdr_len=0,.queue_mapping=0,.__cloned_offset=[],.cloned=0x0,.nohdr=0x0,.fclone=0x0,.peeked=0x0,.head_frag=0x0,.pfmemalloc=0x0,.active_extensions=0,.headers_start=[],.__pkt_type_offset=[],.pkt_type=0x0,.ignore_df=0x0,.nf_trace=0x0,.ip_summed=0x0,.ooo_okay=0x0,.l4_hash=0x0,.sw_hash=0x0,.wifi_acked_valid=0x0,.wifi_acked=0x0,.no_fcs=0x0,.encapsulation=0x0,.encap_hdr_csum=0x0,.csum_valid=0x0,.__pkt_vlan_present_offset=[],.vlan_present=0x0,.csum_complete_sw=0x0,.csum_level=0x0,.csum_not_inet=0x0,.dst_pending_co
> 
> printk output is truncated at 1024 bytes.  For such cases, the compact
> display mode (minus the field info) may be used. "|" differentiates
> between different union members.
> 
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---
>  Documentation/core-api/printk-formats.rst |   8 ++
>  include/linux/btf.h                       |   3 +-
>  lib/Kconfig                               |  16 ++++
>  lib/vsprintf.c                            | 145 +++++++++++++++++++++++++++++-
>  4 files changed, 169 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
> index 8ebe46b1..b786577 100644
> --- a/Documentation/core-api/printk-formats.rst
> +++ b/Documentation/core-api/printk-formats.rst
> @@ -545,6 +545,14 @@ For printing netdev_features_t.
>  
>  Passed by reference.
>  
> +BTF-based printing of pointer data
> +----------------------------------
> +If '%pT[N]<type_name>' is specified, use the BPF Type Format (BTF) to
> +show the typed data.  For example, specifying '%pT<struct sk_buff>' will utilize
> +BTF information to traverse the struct sk_buff * and display it.
> +
> +Supported modifer is 'N' (show type field names).
> +
>  Thanks
>  ======
>  
> diff --git a/include/linux/btf.h b/include/linux/btf.h
> index 2f78dc8..456bd8f 100644
> --- a/include/linux/btf.h
> +++ b/include/linux/btf.h
> @@ -158,10 +158,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/lib/Kconfig b/lib/Kconfig
> index bc7e563..e92109e 100644
> --- a/lib/Kconfig
> +++ b/lib/Kconfig
> @@ -6,6 +6,22 @@
>  config BINARY_PRINTF
>  	def_bool n
>  
> +config BTF_PRINTF

I don't see any IS_ENABLED(BTF_PRINTF) anywhere in this patch? Shouldn't
the vsprintf.c handler be guarded by that?

> +#define is_btf_fmt_start(c)	(c == 'T')
> +#define is_btf_type_start(c)	(c == '<')
> +#define is_btf_type_end(c)	(c == '>')
> +
> +#define btf_modifier_flag(c)	(c == 'N' ? BTF_SHOW_NAME : 0)
> +
> +static noinline_for_stack
> +const char *skip_btf_type(const char *fmt, bool *found_btf_type)
> +{
> +	*found_btf_type = false;
> +
> +	if (!is_btf_fmt_start(*fmt))
> +		return fmt;
> +	fmt++;
> +
> +	while (btf_modifier_flag(*fmt))
> +		fmt++;
> +
> +	if (!is_btf_type_start(*fmt))
> +		return fmt;
> +
> +	while (!is_btf_type_end(*fmt) && *fmt != '\0')
> +		fmt++;
> +
> +	if (is_btf_type_end(*fmt)) {
> +		fmt++;
> +		*found_btf_type = true;
> +	}
> +
> +	return fmt;
> +}
> +
> +static noinline_for_stack
> +char *btf_string(char *buf, char *end, void *ptr, struct printf_spec spec,
> +		 const char *fmt)
> +{
> +	const struct btf_type *btf_type;
> +	char btf_name[KSYM_SYMBOL_LEN];

That seems to be a rather arbitrary size.

> +	u8 btf_kind = BTF_KIND_TYPEDEF;
> +	const struct btf *btf;
> +	char *buf_start = buf;
> +	u64 flags = 0, mod;
> +	s32 btf_id;
> +	int i;
> +
> +	/*
> +	 * Accepted format is [format_modifiers]*<type> ;
> +	 * for example "%pTN<struct sk_buff>" will show a representation
> +	 * of the sk_buff pointed to by the associated argument including
> +	 * member names.
> +	 */
> +	if (check_pointer(&buf, end, ptr, spec))
> +		return buf;
> +
> +	while (isalpha(*fmt)) {
> +		mod = btf_modifier_flag(*fmt);
> +		if (!mod)
> +			break;
> +		flags |= mod;
> +		fmt++;
> +	}
> +
> +	if (!is_btf_type_start(*fmt))
> +		return error_string(buf, end, "(%pT?)", spec);
> +	fmt++;
> +
> +	if (isspace(*fmt))
> +		fmt = skip_spaces(++fmt);

Why not just "fmt = skip_spaces(fmt);"? But actually, why would you want
to support arbitrary whitespace at all? Surely "%pT< struct  abc  >" is
a programmer error.

> +	if (strncmp(fmt, "struct ", strlen("struct ")) == 0) {
> +		btf_kind = BTF_KIND_STRUCT;
> +		fmt += strlen("struct ");
> +	} else if (strncmp(fmt, "union ", strlen("union ")) == 0) {
> +		btf_kind = BTF_KIND_UNION;
> +		fmt += strlen("union ");
> +	} else if (strncmp(fmt, "enum ", strlen("enum ")) == 0) {
> +		btf_kind = BTF_KIND_ENUM;
> +		fmt += strlen("enum ");
> +	}
> +
> +	if (isspace(*fmt))
> +		fmt = skip_spaces(++fmt);
> +
> +	for (i = 0; isalnum(*fmt) || *fmt == '_'; fmt++, i++)
> +		btf_name[i] = *fmt;

So what ensures btf_name is big enough? It's more robust to just store
the starting value of fmt, fast-forward fmt over alnums, compute the
length since the start, bail if too big, otherwise memcpy to btf_name.

> +	btf_name[i] = '\0';
> +
> +	if (isspace(*fmt))
> +		fmt = skip_spaces(++fmt);

Please don't.

> +	if (strlen(btf_name) == 0 || !is_btf_type_end(*fmt))
> +		return error_string(buf, end, "(%pT?)", spec);
> +
> +	btf = bpf_get_btf_vmlinux();
> +	if (IS_ERR_OR_NULL(btf))
> +		return ptr_to_id(buf, end, ptr, spec);
> +
> +	/*
> +	 * Assume type specified is a typedef as there's not much
> +	 * benefit in specifying %p<int> 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_name, btf_kind);
> +	if (btf_id < 0)
> +		btf_id = btf_find_by_name_kind(btf, btf_name,
> +					       BTF_KIND_INT);
> +
> +	if (btf_id >= 0)
> +		btf_type = btf_type_by_id(btf, btf_id);
> +	if (btf_id < 0 || !btf_type)
> +		return ptr_to_id(buf, end, ptr, spec);

That seems like a lot of work to have to do. I'm wondering if the
compiler can't help us in some way (but I know nothing about BTF, so
pardon my ignorance), given that the type printed is known by the
caller. What I'm thinking of is having some kind of

struct pT_arg { int cookie; void *p; }

#define pT_arg(p) &(struct pT_arg) { .cookie =
magic_compiler_thing(typeof(p)), .p = p}

printk("%pT", pT_arg(p));

Even if that can't be done, you could consider using that scheme for
passing the "struct foo_bar" string instead of doing the <> parsing,
i.e. the "cookie" above would just be a "const char *", and the pT_arg()
macro would be called as

pT_arg("struct sk_buff", skb).

Or, better yet, make that pT_arg(struct sk_buff, skb), use
stringification to create the const char* argument, but also add some
BUILD_BUG_ON(!(same_type(t *, typeof(p)) || same_type(const t *,
typeof(p))).

> +	buf += btf_type_snprintf_show(btf, btf_id, ptr, buf,
> +				      end - buf_start, flags);

Does that btf_type_snprintf_show() helper do the right thing when given
a negative or too-small buffer size? From a quick look at patch 3, I see
two problems in btf_snprintf_show():

+	if (ssnprintf->len < 0)
+		return;

That early returns seems to imply that we never produce the "what would
be printed" in case we're already past the end of the buffer.

+	if (len < 0) {
+		ssnprintf->len_left = 0;
+		ssnprintf->len = len;

Testing the return value from snprintf() for being negative is always wrong.


> +	return widen_string(buf, buf - buf_start, end, spec);

Well, ok, but I highly doubt anyone is going to pass a field_width to %pT.

> +}
> +
>  /*
>   * 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 +2291,15 @@ 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[N<type_name>]' For printing pointer data using BPF Type Format (BTF).
> + *
> + *			Optional arguments are
> + *			N		print type and member names
> + *
> + *			Required options are
> + *			<type_name>	associated pointer is interpreted
> + *					to point at type_name.
> + *
>   * ** When making changes please also update:
>   *	Documentation/core-api/printk-formats.rst
>   *
> @@ -2251,6 +2382,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 */
> @@ -2506,6 +2639,7 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
>  	unsigned long long num;
>  	char *str, *end;
>  	struct printf_spec spec = {0};
> +	bool found_btf_type;
>  
>  	/* Reject out-of-range values early.  Large positive sizes are
>  	   used for unknown buffer sizes. */
> @@ -2577,8 +2711,15 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
>  		case FORMAT_TYPE_PTR:
>  			str = pointer(fmt, str, end, va_arg(args, void *),
>  				      spec);
> -			while (isalnum(*fmt))
> -				fmt++;
> +			/*
> +			 * BTF type info is enclosed <like this>, so can
> +			 * contain whitespace.
> +			 */
> +			fmt = skip_btf_type(fmt, &found_btf_type);
> +			if (!found_btf_type) {
> +				while (isalnum(*fmt))
> +					fmt++;
> +			}

As indicated above, this (or the helpers) wants some dependency on
CONFIG_BTF_PRINTF.

Rasmus

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

[Rasmus Villemoes]

On 20/04/2020 18.32, Joe Perches wrote:
> On Mon, 2020-04-20 at 16:29 +0100, Alan Maguire wrote:

>>>>   struct sk_buff *skb = alloc_skb(64, GFP_KERNEL);
>>>>
>>>>   pr_info("%pTN<struct sk_buff>", skb);
>>>
>>> why follow "TN" convention?
>>> I think "%p<struct sk_buff>" is much more obvious, unambiguous, and
>>> equally easy to parse.
>>>
>>
>> That was my first choice, but the first character
>> after the 'p' in the '%p' specifier signifies the
>> pointer format specifier. If we use '<', and have
>> '%p<', where do we put the modifiers? '%p<xYz struct foo>'
>> seems clunky to me.

There's also the issue that %p followed by alnum has been understood to
be reserved/specially handled by the kernel's printf implementation for
a decade, while other characters have so far been treated as "OK, this
was just a normal %p". A quick grep for %p< only gives a hit in
drivers/scsi/dc395x.c, but there could be others (with field width
modifier between % and p), and in any case I think it's a bad idea to
extend the set of characters that cannot follow %p.

Rasmus

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

[Alan Maguire]

On Mon, 20 Apr 2020, Joe Perches wrote:

> On Mon, 2020-04-20 at 16:29 +0100, Alan Maguire wrote:
> > On Sat, 18 Apr 2020, Alexei Starovoitov wrote:
> > 
> > > On Fri, Apr 17, 2020 at 11:42:34AM +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 RFC patchset proposes introducing a "print typed pointer" format
> > > > specifier "%pT<type>"; the type specified is then looked up in the BPF
> > > > Type Format (BTF) information provided for vmlinux to support display.
> > > 
> > > This is great idea! Love it.
> > > 
> > 
> > Thanks for taking a look!
> >  
> > > > 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.
> > > 
> > > yep. This is useful for general purpose printk.
> > > The only piece that must be highlighted in the printk documentation
> > > that unlike the rest of BPF there are zero safety guarantees here.
> > > The programmer can pass wrong pointer to printk() and the kernel _will_ crash.
> > > 
> > 
> > Good point; I'll highlight the fact that we aren't
> > executing in BPF context, no verifier etc.
> > 
> > > >   struct sk_buff *skb = alloc_skb(64, GFP_KERNEL);
> > > > 
> > > >   pr_info("%pTN<struct sk_buff>", skb);
> > > 
> > > why follow "TN" convention?
> > > I think "%p<struct sk_buff>" is much more obvious, unambiguous, and
> > > equally easy to parse.
> > > 
> > 
> > That was my first choice, but the first character
> > after the 'p' in the '%p' specifier signifies the
> > pointer format specifier. If we use '<', and have
> > '%p<', where do we put the modifiers? '%p<xYz struct foo>'
> > seems clunky to me.
> 
> While I don't really like the %p<struct type> block,
> it's at least obvious what's being attempted.
> 
> Modifiers could easily go after the <struct type> block.
>

Good idea; I'll go with that approach for v2.
 
> It appears a %p<struct type> output might be a lot of
> total characters so another potential issue might be
> the maximum length of each individual printk.
>

Right, that's a concern. On the other side, it turns out
that with the "omit zeroed values" behaviour by default,
the output trims down nicely.  The omit zero functionality
works for nested types too, so a freshly-allocated skb
fits easily inside the printk limit now.  The in-progress
output looks like this now (v2 coming shortly I hope):

printk(KERN_INFO "%p<struct sk_buff>", skb);

(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,
  },
 },
}

Of course as structures get used and values get set
(precisely when we need to see them!) more fields will
be displayed and we will bump against printk limits.

The modifiers I'm working on for v2 are

'c' - compact mode (no pretty-printing), i.e.

(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,},},}

      This saves a fair few characters, especially for highly-indented
      data structures.

'T' - omit type/member names.
'x' - avoid pointer obfuscation
'0' - show zeroed values, as suggested by Arnaldo and Alexei

...so the default output of "%p<struct sk_buff>"
will be like the above example.

I was hoping to punt on pointer obfuscation and just
use %p[K] since obfuscation can be sysctl-controlled;
however that control assumes a controlling process context
as I understand it.  Since BTF printk can be invoked
anywhere (especially via trace_printk() in BPF programs),
those settings aren't hugely relevant, so I _think_ we need
a way to directly control obfuscation for this use case.

> > > I like the choice of C style output, but please format it similar to drgn. Like:
> > > *(struct task_struct *)0xffff889ff8a08000 = {
> > > 	.thread_info = (struct thread_info){
> > > 		.flags = (unsigned long)0,
> > > 		.status = (u32)0,
> > > 	},
> > > 	.state = (volatile long)1,
> > > 	.stack = (void *)0xffffc9000c4dc000,
> > > 	.usage = (refcount_t){
> > > 		.refs = (atomic_t){
> > > 			.counter = (int)2,
> > > 		},
> > > 	},
> > > 	.flags = (unsigned int)4194560,
> > > 	.ptrace = (unsigned int)0,
> 
> And here, the issue might be duplicating the log level
> for each line of output and/or prefixing each line with
> whatever struct type is being dumped as interleaving
> with other concurrent logging is possible.
> 

That's a good idea but sadly it makes the problem of longer
display worse.  Compact/type-omitted options help for
the particularly bad cases at least I suppose.

> Here as well the individual field types don't contain
> enough information to determine if a field should be
> output as %x or %u.
> 
>

Right, we could add some more format modifiers for cases
like that I guess.  Currently the display formats used are

- numbers are represented as decimal
- bitfields are represented in hex
- pointers are obfuscated unless the 'x' option is used
- char arrays are printed as chars if printable,
  [ 'l', 'i', 'k', 'e', ' ', 't', 'h', 'i', 's' ]

I'd be happy to add more format specifiers to control
these options, or tweak the defaults if needed. A
"print numbers in hex" option seems worthwhile perhaps?

Thanks for the feedback!

Alan

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

[Joe Perches]

On Thu, 2020-04-30 at 03:03 -0700, Alan Maguire wrote:
> On Mon, 20 Apr 2020, Joe Perches wrote:

> > Here as well the individual field types don't contain
> > enough information to determine if a field should be
> > output as %x or %u.
> Right, we could add some more format modifiers for cases
> like that I guess.  Currently the display formats used are
> - numbers are represented as decimal
> - bitfields are represented in hex
> - pointers are obfuscated unless the 'x' option is used
> - char arrays are printed as chars if printable,
>   [ 'l', 'i', 'k', 'e', ' ', 't', 'h', 'i', 's' ]
> 
> I'd be happy to add more format specifiers to control
> these options, or tweak the defaults if needed. A
> "print numbers in hex" option seems worthwhile perhaps?

Or maybe add and use new typedefs like x8,x16,x32,x64 to the
bpf struct definitions where u8,u16,u32,u64 are %u and
x8,x16,x32,x64 are %x