| 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: <CAEf4BzZ1uFeY1YgL1t5Rcp60a_gXZ0ap3_8=ZOaP9G98_CXfow@mail.gmail.com>
Date: Mon, 30 Sep 2024 16:17:37 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: 燕青洲(Eric Yan) <eric.yan@...o.com>
Cc: Alexei Starovoitov <alexei.starovoitov@...il.com>, kbuild test robot <lkp@...el.com>,
Andrii Nakryiko <andrii@...nel.org>, Alexei Starovoitov <ast@...nel.org>, bpf <bpf@...r.kernel.org>,
Daniel Borkmann <daniel@...earbox.net>, Hao Luo <haoluo@...gle.com>,
John Fastabend <john.fastabend@...il.com>, Jiri Olsa <jolsa@...nel.org>,
KP Singh <kpsingh@...nel.org>, LKML <linux-kernel@...r.kernel.org>,
Martin KaFai Lau <martin.lau@...ux.dev>,
"oe-kbuild-all@...ts.linux.dev" <oe-kbuild-all@...ts.linux.dev>, Stanislav Fomichev <sdf@...ichev.me>,
Song Liu <song@...nel.org>, Yonghong Song <yonghong.song@...ux.dev>
Subject: Re: [PATCH v2] Add BPF Kernel Function bpf_ptrace_vprintk
On Mon, Sep 30, 2024 at 1:29 AM 燕青洲(Eric Yan) <eric.yan@...o.com> wrote:
>
> This patch is mainly considered based on the Android Perfetto (A powerful trace collection and analysis tool, support ftrace data source).
> The output of bpf_trace_printk and bpf_vtrace_printk in ftrace is like:
> app-12345 [001] d... 654321.1970001: bpf_trace_printk: blabla..
>
> FUNCTION field of this kind of message is 'bpf_trace_printk', and there's no standard syntax format for it.
> Currently, Perfetto doesn't collect 'bpf_trace/bpf_trace_printk' trace event by default, but does support
> 'tracing_mark_write' function style by default, such as:
> app-3151 [000] d.h1. 6059.904239: tracing_mark_write: B|2491|BPRF-3151|TracingFunc
> app-3151 [000] d.h1. 6059.904239: tracing_mark_write: E|2491
>
> Therefore, it's considered to add this kfunc to output formatted BPF messages to ftrace like trace_marker,
> allowing perfetto to collect and parse 'tracing_mark_write' events by default and eventually visualize them in the perfetto UI.
This does seem like a bit of an overkill to add a new kfunc just to
have "tracing_mark_write" instead of "bpf_trace_printk". Is there any
chance that perfetto can be changed to also track bpf_trace_printk,
perhaps with some pre-agreed upon prefix or something? E.g,
app-3151 [000] d.h1. 6059.904239: bpf_trace_printk:
!B|2491|BPRF-3151|TracingFunc
app-3151 [000] d.h1. 6059.904239: bpf_trace_printk: !E|2491
Generally speaking, bpf_trace_printk() shouldn't be used in production
setup (much), so perhaps parsing everything from bpf_trace_printk() is
OK (assuming it follows this vertical bar syntax)?
>
> -----邮件原件-----
> 发件人: Alexei Starovoitov <alexei.starovoitov@...il.com>
> 发送时间: 2024年9月30日 1:10
> 收件人: 燕青洲(Eric Yan) <eric.yan@...o.com>
> 抄送: kbuild test robot <lkp@...el.com>; Andrii Nakryiko <andrii@...nel.org>; Alexei Starovoitov <ast@...nel.org>; bpf <bpf@...r.kernel.org>; Daniel Borkmann <daniel@...earbox.net>; Hao Luo <haoluo@...gle.com>; John Fastabend <john.fastabend@...il.com>; Jiri Olsa <jolsa@...nel.org>; KP Singh <kpsingh@...nel.org>; LKML <linux-kernel@...r.kernel.org>; Martin KaFai Lau <martin.lau@...ux.dev>; oe-kbuild-all@...ts.linux.dev; Stanislav Fomichev <sdf@...ichev.me>; Song Liu <song@...nel.org>; Yonghong Song <yonghong.song@...ux.dev>
> 主题: Re: [PATCH v2] Add BPF Kernel Function bpf_ptrace_vprintk
>
> On Thu, Sep 26, 2024 at 12:28 AM Eric Yan <eric.yan@...o.com> wrote:
> >
> > add a kfunc 'bpf_ptrace_vprintk' printing bpf msg with trace_marker
> > format requirement so that these msgs can be retrieved by android
> > perfetto by default and well represented in perfetto UI.
> >
> > [testing prog]
> > const volatile bool ptrace_enabled = true; extern int
> > bpf_ptrace_vprintk(char *fmt, u32 fmt_size, const void *args, u32
> > args__sz) __ksym;
> >
> > ({ \
> > if (!ptrace_enabled) { \
> > bpf_printk(fmt, __VA_ARGS__); \
> > } else { \
> > char __fmt[] = fmt; \
> > _Pragma("GCC diagnostic push") \
> > _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
> > u64 __params[] = { __VA_ARGS__ }; \
> > _Pragma("GCC diagnostic pop") \
> > bpf_ptrace_vprintk(__fmt, sizeof(__fmt), __params, sizeof(__params)); \
> > } \
> > })
> >
> > SEC("perf_event")
> > int do_sample(struct bpf_perf_event_data *ctx) {
> > u64 ip = PT_REGS_IP(&ctx->regs);
> > u64 id = bpf_get_current_pid_tgid();
> > s32 pid = id >> 32;
> > s32 tid = id;
> > debug_printk("N|%d|BPRF-%d|BPRF:%llx", pid, tid, ip);
> > return 0;
> > }
> >
> > [output]:
> > app-3151 [000] d.h1. 6059.904239: tracing_mark_write: N|2491|BPRF-3151|BPRF:58750d0eec
> >
> > Signed-off-by: Eric Yan <eric.yan@...o.com>
> > ---
> > kernel/bpf/helpers.c | 34 ++++++++++++++++++++++++++++++++++
> > 1 file changed, 34 insertions(+)
> >
> > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c index
> > 1a43d06eab28..1e37dae74ca6 100644
> > --- a/kernel/bpf/helpers.c
> > +++ b/kernel/bpf/helpers.c
> > @@ -2521,6 +2521,39 @@ __bpf_kfunc struct task_struct *bpf_task_from_pid(s32 pid)
> > return p;
> > }
> >
> > +static noinline void tracing_mark_write(char *buf) {
> > + trace_printk(buf);
> > +}
> > +
> > +/* same as bpf_trace_vprintk, only with a trace_marker format
> > +requirement
> > + * @fmt: Format string, e.g. <B|E|C|N>|<%d:pid>|<%s:TAG>...
> > + */
> > +__bpf_kfunc int bpf_ptrace_vprintk(char *fmt, u32 fmt_size, const
> > +void *args, u32 args__sz) {
> > + struct bpf_bprintf_data data = {
> > + .get_bin_args = true,
> > + .get_buf = true,
> > + };
> > + int ret, num_args;
> > +
> > + if (args__sz & 7 || args__sz > MAX_BPRINTF_VARARGS * 8 || (args__sz && !args))
> > + return -EINVAL;
> > + num_args = args__sz / 8;
> > +
> > + ret = bpf_bprintf_prepare(fmt, fmt_size, args, num_args, &data);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = bstr_printf(data.buf, MAX_BPRINTF_BUF, fmt,
> > + data.bin_args);
> > +
> > + tracing_mark_write(data.buf);
> > +
> > + bpf_bprintf_cleanup(&data);
> > +
> > + return ret;
> > +}
> > +
> > /**
> > * bpf_dynptr_slice() - Obtain a read-only pointer to the dynptr data.
> > * @p: The dynptr whose data slice to retrieve @@ -3090,6 +3123,7 @@
> > BTF_ID_FLAGS(func, bpf_iter_bits_new, KF_ITER_NEW) BTF_ID_FLAGS(func,
> > bpf_iter_bits_next, KF_ITER_NEXT | KF_RET_NULL) BTF_ID_FLAGS(func,
> > bpf_iter_bits_destroy, KF_ITER_DESTROY) BTF_ID_FLAGS(func,
> > bpf_copy_from_user_str, KF_SLEEPABLE)
> > +BTF_ID_FLAGS(func, bpf_ptrace_vprintk)
> > BTF_KFUNCS_END(common_btf_ids)
>
> Why new kfunc?
> Use bpf_snprintf() and follow with bpf_trace_printk() ?
Powered by blists - more mailing lists