| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20200623124012.GV2428291@smile.fi.intel.com>
Date: Tue, 23 Jun 2020 15:40:12 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Alan Maguire <alan.maguire@...cle.com>
Cc: ast@...nel.org, daniel@...earbox.net, yhs@...com, andriin@...com,
arnaldo.melo@...il.com, kafai@...com, songliubraving@...com,
john.fastabend@...il.com, kpsingh@...omium.org,
linux@...musvillemoes.dk, joe@...ches.com, pmladek@...e.com,
rostedt@...dmis.org, sergey.senozhatsky@...il.com, corbet@....net,
bpf@...r.kernel.org, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org
Subject: Re: [PATCH v3 bpf-next 4/8] printk: add type-printing %pT format
specifier which uses BTF
On Tue, Jun 23, 2020 at 01:07:07PM +0100, Alan Maguire wrote:
> printk supports multiple pointer object type specifiers (printing
> netdev features etc). Extend this support using BTF to cover
> arbitrary types.
Is there any plans to cover (all?) existing %p extensions?
> "%pT"
One letter namespace is quite busy area. Perhaps %pOT ?
> 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 *)0x000000006b71155a,
> .data = (unsigned char *)0x000000006b71155a,
> .truesize = (unsigned int)768,
> .users = (refcount_t){
> .refs = (atomic_t){
> .counter = (int)1,
> },
> },
> .extensions = (struct skb_ext *)0x00000000f486a130,
> }
I don't see how it looks on a real console when kernel dumps something.
Care to provide? These examples better to have documented.
> printk output is truncated at 1024 bytes. For cases where overflow
> is likely, the compact/no type names display modes may be used.
How * is handled? (I mean %*pOT case)
...
> +#define BTF_PTR_TYPE(ptrval, typeval) \
> + (&((struct btf_ptr){.ptr = ptrval, .type = #typeval}))
> +
> +#define BTF_PTR_ID(ptrval, idval) \
> + (&((struct btf_ptr){.ptr = ptrval, .id = idval}))
Wouldn't be better if these will leave in its own (linker) section?
...
> +static noinline_for_stack
> +char *btf_string(char *buf, char *end, void *ptr, struct printf_spec spec,
> + const char *fmt)
> +{
> + struct btf_ptr *bp = (struct btf_ptr *)ptr;
Unneeded casting.
> + u8 btf_kind = BTF_KIND_TYPEDEF;
> + const struct btf_type *t;
> + const struct btf *btf;
> + char *buf_start = buf;
> + const char *btf_type;
> + u64 flags = 0, mod;
> + s32 btf_id;
> +
> + if (check_pointer(&buf, end, ptr, spec))
> + return buf;
> +
> + if (check_pointer(&buf, end, bp->ptr, spec))
> + return buf;
> + while (isalnum(*fmt)) {
> + mod = btf_modifier_flag(*fmt);
> + if (!mod)
> + break;
> + flags |= mod;
> + fmt++;
> + }
Can't we have explicitly all handled flags here, like other extensions do?
> + btf = bpf_get_btf_vmlinux();
> + if (IS_ERR_OR_NULL(btf))
> + return ptr_to_id(buf, end, bp->ptr, spec);
> +
> + if (bp->type != NULL) {
> + btf_type = bp->type;
> +
> + if (strncmp(bp->type, "struct ", strlen("struct ")) == 0) {
> + btf_kind = BTF_KIND_STRUCT;
> + btf_type += strlen("struct ");
> + } else if (strncmp(btf_type, "union ", strlen("union ")) == 0) {
> + btf_kind = BTF_KIND_UNION;
> + btf_type += strlen("union ");
> + } else if (strncmp(btf_type, "enum ", strlen("enum ")) == 0) {
> + btf_kind = BTF_KIND_ENUM;
> + btf_type += strlen("enum ");
> + }
Can't you provide a simple structure and do this in a loop?
Or even something like match_[partial]string() to implement?
> + if (strlen(btf_type) == 0)
Interesting way of checking btf_type == '\0'.
> + return ptr_to_id(buf, end, bp->ptr, spec);
> +
> + /*
> + * Assume type specified is a typedef as there's not much
> + * benefit in specifying int types other than wasting time
> + * on BTF lookups; we optimize for the most useful path.
> + *
> + * Fall back to BTF_KIND_INT if this fails.
> + */
> + btf_id = btf_find_by_name_kind(btf, btf_type, btf_kind);
> + if (btf_id < 0)
> + btf_id = btf_find_by_name_kind(btf, btf_type,
> + BTF_KIND_INT);
> + } else if (bp->id > 0)
> + btf_id = bp->id;
> + else
> + return ptr_to_id(buf, end, bp->ptr, spec);
> +
> + if (btf_id > 0)
> + t = btf_type_by_id(btf, btf_id);
> + if (btf_id <= 0 || !t)
> + return ptr_to_id(buf, end, bp->ptr, spec);
This can be easily incorporated in previous conditional tree.
> + buf += btf_type_snprintf_show(btf, btf_id, bp->ptr, buf,
> + end - buf_start, flags);
> +
> + return widen_string(buf, buf - buf_start, end, spec);
> +}
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists