Re: [PATCH v2 bpf-next 4/7] printk: add type-printing %pT format specifier which uses BTF
From: Joe Perches
Date: Thu May 14 2020 - 19:43:34 EST
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@xxxxxxxxxxx> 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