All of lore.kernel.org
 help / color / mirror / Atom feed
From: Yonghong Song <yhs@fb.com>
To: Alan Maguire <alan.maguire@oracle.com>, <ast@kernel.org>,
	<daniel@iogearbox.net>, <bpf@vger.kernel.org>
Cc: <joe@perches.com>, <linux@rasmusvillemoes.dk>,
	<arnaldo.melo@gmail.com>, <kafai@fb.com>, <songliubraving@fb.com>,
	<andriin@fb.com>, <john.fastabend@gmail.com>,
	<kpsingh@chromium.org>, <linux-kernel@vger.kernel.org>,
	<netdev@vger.kernel.org>
Subject: Re: [PATCH v2 bpf-next 6/7] bpf: add support for %pT format specifier for bpf_trace_printk() helper
Date: Wed, 13 May 2020 17:53:42 -0700	[thread overview]
Message-ID: <040b71a1-9bbf-9a55-6f1a-e7b8c36f8c6e@fb.com> (raw)
In-Reply-To: <1589263005-7887-7-git-send-email-alan.maguire@oracle.com>



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

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


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

  reply	other threads:[~2020-05-14  0:54 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-12  5:56 [PATCH v2 bpf-next 0/7] bpf, printk: add BTF-based type printing Alan Maguire
2020-05-12  5:56 ` [PATCH v2 bpf-next 1/7] bpf: provide function to get vmlinux BTF information Alan Maguire
2020-05-12  5:56 ` [PATCH v2 bpf-next 2/7] bpf: move to generic BTF show support, apply it to seq files/strings Alan Maguire
2020-05-13 23:04   ` Yonghong Song
2020-05-18  9:46     ` Alan Maguire
2020-05-19  6:21       ` Yonghong Song
2020-05-12  5:56 ` [PATCH v2 bpf-next 3/7] checkpatch: add new BTF pointer format specifier Alan Maguire
2020-05-12  5:56 ` [PATCH v2 bpf-next 4/7] printk: add type-printing %pT format specifier which uses BTF Alan Maguire
2020-05-13 23:05   ` Joe Perches
2020-05-13 23:07     ` Alexei Starovoitov
2020-05-13 23:22       ` Joe Perches
2020-05-14 23:43         ` Joe Perches
2020-05-15  0:09           ` Alexei Starovoitov
2020-05-15  0:21             ` Joe Perches
2020-05-14  0:45   ` Yonghong Song
2020-05-14 22:37     ` Alan Maguire
2020-05-15  0:39       ` Yonghong Song
2020-05-12  5:56 ` [PATCH v2 bpf-next 5/7] printk: extend test_printf to test %pT BTF-based format specifier Alan Maguire
2020-05-12  5:56 ` [PATCH v2 bpf-next 6/7] bpf: add support for %pT format specifier for bpf_trace_printk() helper Alan Maguire
2020-05-14  0:53   ` Yonghong Song [this message]
2020-05-18  9:10     ` Alan Maguire
2020-05-18 14:47       ` Yonghong Song
2020-05-12  5:56 ` [PATCH v2 bpf-next 7/7] bpf: add tests for %pT format specifier Alan Maguire
2020-05-15  0:21   ` Andrii Nakryiko
2020-05-13 22:24 ` [PATCH v2 bpf-next 0/7] bpf, printk: add BTF-based type printing Alexei Starovoitov
2020-05-13 22:48   ` Joe Perches
2020-05-13 22:50     ` Alexei Starovoitov
2020-05-13 23:23       ` Joe Perches
2020-05-14 17:46   ` Alan Maguire
2020-05-15 18:59   ` Yonghong Song

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=040b71a1-9bbf-9a55-6f1a-e7b8c36f8c6e@fb.com \
    --to=yhs@fb.com \
    --cc=alan.maguire@oracle.com \
    --cc=andriin@fb.com \
    --cc=arnaldo.melo@gmail.com \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=joe@perches.com \
    --cc=john.fastabend@gmail.com \
    --cc=kafai@fb.com \
    --cc=kpsingh@chromium.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@rasmusvillemoes.dk \
    --cc=netdev@vger.kernel.org \
    --cc=songliubraving@fb.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.