From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752031AbbCTVW1 (ORCPT ); Fri, 20 Mar 2015 17:22:27 -0400 Received: from smtprelay0021.hostedemail.com ([216.40.44.21]:42707 "EHLO smtprelay.hostedemail.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751717AbbCTVWX (ORCPT ); Fri, 20 Mar 2015 17:22:23 -0400 X-Session-Marker: 726F737465647440676F6F646D69732E6F7267 X-Spam-Summary: 2,0,0,,d41d8cd98f00b204,rostedt@goodmis.org,:::::::::::::::::::::::,RULES_HIT:41:355:379:541:599:800:960:973:988:989:1260:1277:1311:1313:1314:1345:1359:1431:1437:1515:1516:1518:1535:1544:1593:1594:1605:1711:1730:1747:1777:1792:2198:2199:2393:2553:2559:2562:2693:2898:2901:3138:3139:3140:3141:3142:3622:3865:3866:3867:3868:3871:3872:3874:4250:4321:4605:5007:6119:6120:6261:6742:7875:7901:7903:8603:8660:9040:10004:10848:10967:11026:11232:11473:11658:11914:12043:12291:12296:12438:12517:12519:12555:12683:12740:13148:13230:14096:14097:21080,0,RBL:none,CacheIP:none,Bayesian:0.5,0.5,0.5,Netcheck:none,DomainCache:0,MSF:not bulk,SPF:fn,MSBL:0,DNSBL:none,Custom_rules:0:0:0 X-HE-Tag: mice35_4eac6cad4b356 X-Filterd-Recvd-Size: 5987 Date: Fri, 20 Mar 2015 17:22:19 -0400 From: Steven Rostedt To: Alexei Starovoitov Cc: Ingo Molnar , Namhyung Kim , Arnaldo Carvalho de Melo , Jiri Olsa , Masami Hiramatsu , "David S. Miller" , Daniel Borkmann , Peter Zijlstra , linux-api@vger.kernel.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v8 tip 5/9] tracing: allow BPF programs to call bpf_trace_printk() Message-ID: <20150320172219.45ff7157@gandalf.local.home> In-Reply-To: <1426816787-5001-6-git-send-email-ast@plumgrid.com> References: <1426816787-5001-1-git-send-email-ast@plumgrid.com> <1426816787-5001-6-git-send-email-ast@plumgrid.com> X-Mailer: Claws Mail 3.11.1 (GTK+ 2.24.25; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 19 Mar 2015 18:59:43 -0700 Alexei Starovoitov wrote: > Debugging of BPF programs needs some form of printk from the program, > so let programs call limited trace_printk() with %d %u %x %p modifiers only. > > Similar to kernel modules, during program load verifier checks whether program > is calling bpf_trace_printk() and if so, kernel allocates trace_printk buffers > and emits big 'this is debug only' banner. > > Signed-off-by: Alexei Starovoitov > --- > include/uapi/linux/bpf.h | 1 + > kernel/trace/bpf_trace.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 76 insertions(+) > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > index 238c6883877b..cc47ef41076a 100644 > --- a/include/uapi/linux/bpf.h > +++ b/include/uapi/linux/bpf.h > @@ -166,6 +166,7 @@ enum bpf_func_id { > BPF_FUNC_map_delete_elem, /* int map_delete_elem(&map, &key) */ > BPF_FUNC_probe_read, /* int bpf_probe_read(void *dst, int size, void *src) */ > BPF_FUNC_ktime_get_ns, /* u64 bpf_ktime_get_ns(void) */ > + BPF_FUNC_trace_printk, /* int bpf_trace_printk(const char *fmt, int fmt_size, ...) */ > __BPF_FUNC_MAX_ID, > }; > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > index 769c577a1fc8..d5b6284edd75 100644 > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c > @@ -10,6 +10,7 @@ > #include > #include > #include > +#include > #include "trace.h" > > static DEFINE_PER_CPU(int, bpf_prog_active); > @@ -83,6 +84,72 @@ static const struct bpf_func_proto bpf_ktime_get_ns_proto = { > .ret_type = RET_INTEGER, > }; > > +/* limited trace_printk() > + * only %d %u %x %ld %lu %lx %lld %llu %llx %p conversion specifiers allowed > + */ Ah! Again, don't contaminate the rest of the kernel with net comment styles! :-) I change my comment style when I submit net patches. So patches that go into tracing must conform to the tracing style. > +static u64 bpf_trace_printk(u64 r1, u64 fmt_size, u64 r3, u64 r4, u64 r5) > +{ > + char *fmt = (char *) (long) r1; > + int mod[3] = {}; > + int fmt_cnt = 0; > + int i; > + > + /* bpf_check()->check_func_arg()->check_stack_boundary() It spreads like a disease! > + * guarantees that fmt points to bpf program stack, > + * fmt_size bytes of it were initialized and fmt_size > 0 > + */ > + if (fmt[--fmt_size] != 0) > + return -EINVAL; > + > + /* check format string for allowed specifiers */ > + for (i = 0; i < fmt_size; i++) { > + if ((!isprint(fmt[i]) && !isspace(fmt[i])) || !isascii(fmt[i])) > + return -EINVAL; > + > + if (fmt[i] != '%') > + continue; > + > + if (fmt_cnt >= 3) > + return -EINVAL; > + > + /* fmt[i] != 0 && fmt[last] == 0, so we can access fmt[i + 1] */ > + i++; > + if (fmt[i] == 'l') { > + mod[fmt_cnt]++; > + i++; > + } else if (fmt[i] == 'p') { > + mod[fmt_cnt]++; > + i++; > + if (!isspace(fmt[i]) && fmt[i] != 0) I wonder if we should allow punctuation here too? None alpha-numeric characters? > + return -EINVAL; > + fmt_cnt++; > + continue; > + } > + > + if (fmt[i] == 'l') { > + mod[fmt_cnt]++; > + i++; > + } > + > + if (fmt[i] != 'd' && fmt[i] != 'u' && fmt[i] != 'x') > + return -EINVAL; > + fmt_cnt++; > + } > + > + return __trace_printk(1/* fake ip will not be printed */, fmt, > + mod[0] == 2 ? r3 : mod[0] == 1 ? (long) r3 : (u32) r3, > + mod[1] == 2 ? r4 : mod[1] == 1 ? (long) r4 : (u32) r4, > + mod[2] == 2 ? r5 : mod[2] == 1 ? (long) r5 : (u32) r5); Nice trick. Except for the net-contaminated-comments, the rest looks good. Reviewed-by: Steven Rostedt -- Steve > +} > + > +static const struct bpf_func_proto bpf_trace_printk_proto = { > + .func = bpf_trace_printk, > + .gpl_only = true, > + .ret_type = RET_INTEGER, > + .arg1_type = ARG_PTR_TO_STACK, > + .arg2_type = ARG_CONST_STACK_SIZE, > +}; > + > static const struct bpf_func_proto *kprobe_prog_func_proto(enum bpf_func_id func_id) > { > switch (func_id) { > @@ -96,6 +163,14 @@ static const struct bpf_func_proto *kprobe_prog_func_proto(enum bpf_func_id func > return &bpf_probe_read_proto; > case BPF_FUNC_ktime_get_ns: > return &bpf_ktime_get_ns_proto; > + > + case BPF_FUNC_trace_printk: > + /* this program might be calling bpf_trace_printk, > + * so allocate per-cpu printk buffers > + */ > + trace_printk_init_buffers(); > + > + return &bpf_trace_printk_proto; > default: > return NULL; > }