Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1214338 > unrolled thread

[PATCH net-next] bpf: add support for %s specifier to bpf_trace_printk()

Back to article view | Back to linux.kernel

Contents

  [PATCH net-next] bpf: add support for %s specifier to bpf_trace_printk() Alexei Starovoitov <ast@plumgrid.com> - 2015-08-27 08:30 +0200
    Re: [PATCH net-next] bpf: add support for %s specifier to  bpf_trace_printk() David Miller <davem@davemloft.net> - 2015-08-28 00:40 +0200
      Re: [PATCH net-next] bpf: add support for %s specifier to  bpf_trace_printk() Alexei Starovoitov <ast@plumgrid.com> - 2015-08-28 01:10 +0200
        Re: [PATCH net-next] bpf: add support for %s specifier to  bpf_trace_printk() David Miller <davem@davemloft.net> - 2015-08-28 01:30 +0200
          Re: [PATCH net-next] bpf: add support for %s specifier to  bpf_trace_printk() Steven Rostedt <rostedt@goodmis.org> - 2015-08-28 01:50 +0200
            Re: [PATCH net-next] bpf: add support for %s specifier to  bpf_trace_printk() Alexei Starovoitov <ast@plumgrid.com> - 2015-08-28 04:00 +0200

#1214338 — [PATCH net-next] bpf: add support for %s specifier to bpf_trace_printk()

%s specifier makes bpf program and kernel debugging easier.
To make sure that trace_printk won't crash the unsafe string
is copied into stack and unsafe pointer is substituted.
String is also sanitized for printable characters.
The cost of swapping FS in probe_kernel_read is
amortized over 4 chars to improve performance.

Suggested-by: Brendan Gregg <brendan.d.gregg@gmail.com>
Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
---
The following C program:
 #include <linux/fs.h>
int foo(struct pt_regs *ctx, struct filename *filename)
{
  void *name = 0;

  bpf_probe_read(&name, sizeof(name), &filename->name);
  bpf_trace_printk("executed %s\\n", name);
  return 0;
}

when attached to kprobe do_execve()
will produce output in /sys/kernel/debug/tracing/trace_pipe :
    make-13492 [002] d..1  3250.997277: : executed /bin/sh
      sh-13493 [004] d..1  3250.998716: : executed /usr/bin/gcc
     gcc-13494 [002] d..1  3250.999822: : executed /usr/lib/gcc/x86_64-linux-gnu/4.7/cc1
     gcc-13495 [002] d..1  3251.006731: : executed /usr/bin/as
     gcc-13496 [002] d..1  3251.011831: : executed /usr/lib/gcc/x86_64-linux-gnu/4.7/collect2
collect2-13497 [000] d..1  3251.012941: : executed /usr/bin/ld

 kernel/trace/bpf_trace.c |   60 +++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 57 insertions(+), 3 deletions(-)

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index ef9936df1b04..60d8f95258ed 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -79,13 +79,51 @@ static const struct bpf_func_proto bpf_probe_read_proto = {
 	.arg3_type	= ARG_ANYTHING,
 };
 
+static bool is_valid_char(char c)
+{
+	return (isprint(c) || isspace(c)) && isascii(c);
+}
+
+/* similar to strncpy_from_user() but with extra checks */
+static void probe_read_string(char *buf, int size, long unsafe_ptr)
+{
+	char dst[4];
+	int i = 0;
+
+	size--;
+	for (;;) {
+		if (probe_kernel_read(dst, (void *) unsafe_ptr, 4))
+			break;
+
+		unsafe_ptr += 4;
+
+		if (dst[0] == 0 || !is_valid_char(dst[0]) || i >= size)
+			break;
+		buf[i++] = dst[0];
+
+		if (dst[1] == 0 || !is_valid_char(dst[1]) || i >= size)
+			break;
+		buf[i++] = dst[1];
+
+		if (dst[2] == 0 || !is_valid_char(dst[2]) || i >= size)
+			break;
+		buf[i++] = dst[2];
+
+		if (dst[3] == 0 || !is_valid_char(dst[3]) || i >= size)
+			break;
+		buf[i++] = dst[3];
+	}
+	buf[i] = 0;
+}
+
 /*
  * limited trace_printk()
- * only %d %u %x %ld %lu %lx %lld %llu %llx %p conversion specifiers allowed
+ * only %d %u %x %ld %lu %lx %lld %llu %llx %p %s conversion specifiers allowed
  */
 static u64 bpf_trace_printk(u64 r1, u64 fmt_size, u64 r3, u64 r4, u64 r5)
 {
 	char *fmt = (char *) (long) r1;
+	char buf[64];
 	int mod[3] = {};
 	int fmt_cnt = 0;
 	int i;
@@ -100,7 +138,7 @@ static u64 bpf_trace_printk(u64 r1, u64 fmt_size, u64 r3, u64 r4, u64 r5)
 
 	/* check format string for allowed specifiers */
 	for (i = 0; i < fmt_size; i++) {
-		if ((!isprint(fmt[i]) && !isspace(fmt[i])) || !isascii(fmt[i]))
+		if (!is_valid_char(fmt[i]))
 			return -EINVAL;
 
 		if (fmt[i] != '%')
@@ -114,12 +152,28 @@ static u64 bpf_trace_printk(u64 r1, u64 fmt_size, u64 r3, u64 r4, u64 r5)
 		if (fmt[i] == 'l') {
 			mod[fmt_cnt]++;
 			i++;
-		} else if (fmt[i] == 'p') {
+		} else if (fmt[i] == 'p' || fmt[i] == 's') {
 			mod[fmt_cnt]++;
 			i++;
 			if (!isspace(fmt[i]) && !ispunct(fmt[i]) && fmt[i] != 0)
 				return -EINVAL;
 			fmt_cnt++;
+			if (fmt[i - 1] == 's') {
+				switch (fmt_cnt) {
+				case 1:
+					probe_read_string(buf, sizeof(buf), r3);
+					r3 = (long) buf;
+					break;
+				case 2:
+					probe_read_string(buf, sizeof(buf), r4);
+					r4 = (long) buf;
+					break;
+				case 3:
+					probe_read_string(buf, sizeof(buf), r5);
+					r5 = (long) buf;
+					break;
+				}
+			}
 			continue;
 		}
 
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1214920 — Re: [PATCH net-next] bpf: add support for %s specifier to bpf_trace_printk()

From: Alexei Starovoitov <ast@plumgrid.com>
Date: Wed, 26 Aug 2015 23:26:59 -0700

> +/* similar to strncpy_from_user() but with extra checks */
> +static void probe_read_string(char *buf, int size, long unsafe_ptr)
> +{
> +	char dst[4];
> +	int i = 0;
> +
> +	size--;
> +	for (;;) {
> +		if (probe_kernel_read(dst, (void *) unsafe_ptr, 4))
> +			break;

I don't think this does the right thing when the string is not a multiple
of 3 and ends at the last byte of a page that ends a valid region of
kernel memory.

Seeing this kind of error makes me skeptical to the overall value of
optimizing this :-/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1215002 — Re: [PATCH net-next] bpf: add support for %s specifier to bpf_trace_printk()

On 8/27/15 3:34 PM, David Miller wrote:
> From: Alexei Starovoitov <ast@plumgrid.com>
> Date: Wed, 26 Aug 2015 23:26:59 -0700
>
>> +/* similar to strncpy_from_user() but with extra checks */
>> +static void probe_read_string(char *buf, int size, long unsafe_ptr)
>> +{
>> +	char dst[4];
>> +	int i = 0;
>> +
>> +	size--;
>> +	for (;;) {
>> +		if (probe_kernel_read(dst, (void *) unsafe_ptr, 4))
>> +			break;
>
> I don't think this does the right thing when the string is not a multiple
> of 3 and ends at the last byte of a page that ends a valid region of
> kernel memory.
>
> Seeing this kind of error makes me skeptical to the overall value of
> optimizing this :-/

I've considered the case when first two bytes are valid, but the
other two are in a different page. In such case the probe_read_string()
will trim the string and won't be printing these two valid bytes.
I think that's very rare, so I'm picking higher performance for common
case. The strings over > 64 bytes also will be trimmed to 64.
It's a debugging facility, so I felt that's ok.
Fair or you still think it should be per byte copy?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1215006 — Re: [PATCH net-next] bpf: add support for %s specifier to bpf_trace_printk()

From: Alexei Starovoitov <ast@plumgrid.com>
Date: Thu, 27 Aug 2015 16:06:14 -0700

> Fair or you still think it should be per byte copy?

I'm terribly surprised we don't have an equivalent of strncpy()
for unsafe kernel pointers.

You probably won't be the last person to want this, and it's silly
to optimize it in one place and then wait for cut&paste into the
next guy.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1215010 — Re: [PATCH net-next] bpf: add support for %s specifier to bpf_trace_printk()

On Thu, 27 Aug 2015 16:20:39 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Alexei Starovoitov <ast@plumgrid.com>
> Date: Thu, 27 Aug 2015 16:06:14 -0700
> 
> > Fair or you still think it should be per byte copy?
> 
> I'm terribly surprised we don't have an equivalent of strncpy()
> for unsafe kernel pointers.
> 
> You probably won't be the last person to want this, and it's silly
> to optimize it in one place and then wait for cut&paste into the
> next guy.

If it doesn't exist. Perhaps its time to create it.

-- Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1215057 — Re: [PATCH net-next] bpf: add support for %s specifier to bpf_trace_printk()

On 8/27/15 4:43 PM, Steven Rostedt wrote:
> On Thu, 27 Aug 2015 16:20:39 -0700 (PDT)
> David Miller <davem@davemloft.net> wrote:
>
>> From: Alexei Starovoitov <ast@plumgrid.com>
>> Date: Thu, 27 Aug 2015 16:06:14 -0700
>>
>>> Fair or you still think it should be per byte copy?
>>
>> I'm terribly surprised we don't have an equivalent of strncpy()
>> for unsafe kernel pointers.
>>
>> You probably won't be the last person to want this, and it's silly
>> to optimize it in one place and then wait for cut&paste into the
>> next guy.
>
> If it doesn't exist. Perhaps its time to create it.

all makes sense. Working on generalizing FETCH_FUNC_NAME
from trace_kprobe.c. Seems to fit quite well.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web