Dealing with long path names #900

kbukin1 · 2017-01-11T19:58:18Z

I need to ensure my tracer captures full paths passed by users into some system calls. Paths can be pretty long, up-to PATH_MAX chars. In cases when path is longer than some (~255-400 chars) value I would like putting several data structures carrying different parts of the long paths into BPF_PERF_OUTPUT buffer. So I need to find a length of a null-terminated string in a bpf program. The problem is maximum string I can iterate (using “if … goto” trick) is ~500 chars. An attempt to have a longer unrolled loop results in errors like:

bpf: Invalid argument. Program too large (4739 insns), at most 4096 insns

If I try to read 4095 bytes with bpf_probe_read() into a buffer allocated on the stack of bpf function (so all 4095 bytes could be passed to the user afterwards) then I get an error:

…
73: (b7) r2 = 2048
74: (85) call 4
invalid stack type R1 off=-2352 access_size=2048

Do you have any suggestions on how to find length of a long string (up-to PATH_MAX chars) in bpf program or how to pass long paths into user space? String lives in user space. A pointer to the string is passed as an argument into a system call. BPF program which needs string length is attached to the corresponding system call.

I added bpf_strnlen function to the kernel and bcc toolkit (which calls strnlen_user(buf,size)) and that is working as expected, but it would be great to be able to do that on an unmodified kernel. Any chance bpf/bcc will be getting something like bpf_strnlen() anytime soon? bpf_strnlen seems to be straightforward, but if you are interested I could make a pull request.

brendangregg · 2017-01-11T22:44:23Z

You wrote a bpf_strlen()? Would like to see diffs. :)

kbukin1 · 2017-01-12T00:23:07Z

Here is a diff against kernel 4.9.2:

build-test1:linux-4.9.2$git diff
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index c201017..64516be 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -327,6 +327,7 @@ extern const struct bpf_func_proto bpf_get_current_comm_proto;
 extern const struct bpf_func_proto bpf_skb_vlan_push_proto;
 extern const struct bpf_func_proto bpf_skb_vlan_pop_proto;
 extern const struct bpf_func_proto bpf_get_stackid_proto;
+extern const struct bpf_func_proto bpf_strnlen_proto;

 /* Shared helpers among cBPF and eBPF. */
 void bpf_user_rnd_init_once(void);
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index f09c70b..38c6400 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -426,6 +426,16 @@ enum bpf_func_id {
     */
    BPF_FUNC_set_hash_invalid,

+    /**
+     * bpf_strnlen(str, size)
+     * Get the size of a user string INCLUDING final NUL
+     * @str: The string to measure.
+     * @count: Maximum count (including NUL character)
+     *
+     * see comments for strnlen_user(const char __user *str, long count)
+     */
+   BPF_FUNC_strnlen,
+
    __BPF_FUNC_MAX_ID,
 };

diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index aa6d981..ab04742 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -1048,6 +1048,7 @@ const struct bpf_func_proto bpf_ktime_get_ns_proto __weak;
 const struct bpf_func_proto bpf_get_current_pid_tgid_proto __weak;
 const struct bpf_func_proto bpf_get_current_uid_gid_proto __weak;
 const struct bpf_func_proto bpf_get_current_comm_proto __weak;
+const struct bpf_func_proto bpf_strnlen __weak;

 const struct bpf_func_proto * __weak bpf_get_trace_printk_proto(void)
 {
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 3991840..ec64244 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -120,6 +120,17 @@ const struct bpf_func_proto bpf_get_current_pid_tgid_proto = {
    .ret_type   = RET_INTEGER,
 };

+BPF_CALL_2(bpf_strnlen, const char __user *, buf, u32, size)
+{
+    return strnlen_user(buf, size);
+}
+
+const struct bpf_func_proto bpf_strnlen_proto = {
+   .func       = bpf_strnlen,
+   .gpl_only   = false,
+   .ret_type   = RET_INTEGER,
+};
+
 BPF_CALL_0(bpf_get_current_uid_gid)
 {
    struct task_struct *task = current;
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 5dcb992..195f79a 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -416,6 +416,8 @@ static const struct bpf_func_proto *tracing_func_proto(enum bpf_func_id func_id)
        return &bpf_get_current_task_proto;
    case BPF_FUNC_get_current_uid_gid:
        return &bpf_get_current_uid_gid_proto;
+   case BPF_FUNC_strnlen:
+       return &bpf_strnlen_proto;
    case BPF_FUNC_get_current_comm:
        return &bpf_get_current_comm_proto;
    case BPF_FUNC_trace_printk:

Here is a corresponding diff for bcc (my tree is synced to e14519e - (HEAD -> master, origin/master, origin/HEAD) Merge pull request #887 from rneugeba/cflags (4 days ago) ):

build-test1:bcc$git diff src/cc/compat/linux/bpf.h src/cc/compat/linux/virtual_bpf.h src/cc/export/helpers.h
diff --git a/src/cc/compat/linux/bpf.h b/src/cc/compat/linux/bpf.h
index e7bb296..68bf33e 100644
--- a/src/cc/compat/linux/bpf.h
+++ b/src/cc/compat/linux/bpf.h
@@ -474,6 +474,7 @@ union bpf_attr {
    FN(skb_pull_data),      \
    FN(csum_update),        \
    FN(set_hash_invalid),       \
+   FN(strnlen),        \
    FN(get_numa_node_id),       \
    FN(skb_change_head),        \
    FN(xdp_adjust_head),
diff --git a/src/cc/compat/linux/virtual_bpf.h b/src/cc/compat/linux/virtual_bpf.h
index 05348ff..1ae1cb7 100644
--- a/src/cc/compat/linux/virtual_bpf.h
+++ b/src/cc/compat/linux/virtual_bpf.h
@@ -475,6 +475,7 @@ union bpf_attr {
    FN(skb_pull_data),      \
    FN(csum_update),        \
    FN(set_hash_invalid),       \
+   FN(strnlen),        \
    FN(get_numa_node_id),       \
    FN(skb_change_head),        \
    FN(xdp_adjust_head),
diff --git a/src/cc/export/helpers.h b/src/cc/export/helpers.h
index dce8b2c..6393887 100644
--- a/src/cc/export/helpers.h
+++ b/src/cc/export/helpers.h
@@ -158,6 +158,8 @@ static u64 (*bpf_get_current_pid_tgid)(void) =
   (void *) BPF_FUNC_get_current_pid_tgid;
 static u64 (*bpf_get_current_uid_gid)(void) =
   (void *) BPF_FUNC_get_current_uid_gid;
+static int (*bpf_strlen)(const char *buf, int buf_size) =
+  (void *) BPF_FUNC_strnlen;
 static int (*bpf_get_current_comm)(void *buf, int buf_size) =
   (void *) BPF_FUNC_get_current_comm;
 static u64 (*bpf_get_cgroup_classid)(void *ctx) =

4ast · 2017-01-12T00:37:01Z

On Wed, Jan 11, 2017 at 4:23 PM, Konstantin Bukin ***@***.***> wrote: Here is a diff against kernel 4.9.2: build-test1:linux-4.9.2$git diff diff --git a/include/linux/bpf.h b/include/linux/bpf.h index c201017..64516be 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -327,6 +327,7 @@ extern const struct bpf_func_proto bpf_get_current_comm_proto; extern const struct bpf_func_proto bpf_skb_vlan_push_proto; extern const struct bpf_func_proto bpf_skb_vlan_pop_proto; extern const struct bpf_func_proto bpf_get_stackid_proto; +extern const struct bpf_func_proto bpf_strnlen_proto; /* Shared helpers among cBPF and eBPF. */ void bpf_user_rnd_init_once(void); diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index f09c70b..38c6400 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -426,6 +426,16 @@ enum bpf_func_id { */ BPF_FUNC_set_hash_invalid, + /** + * bpf_strnlen(str, size) + * Get the size of a user string INCLUDING final NUL + * @str: The string to measure. + * @count: Maximum count (including NUL character) + * + * see comments for strnlen_user(const char __user *str, long count) + */ + BPF_FUNC_strnlen, + __BPF_FUNC_MAX_ID, }; diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c index aa6d981..ab04742 100644 --- a/kernel/bpf/core.c +++ b/kernel/bpf/core.c @@ -1048,6 +1048,7 @@ const struct bpf_func_proto bpf_ktime_get_ns_proto __weak; const struct bpf_func_proto bpf_get_current_pid_tgid_proto __weak; const struct bpf_func_proto bpf_get_current_uid_gid_proto __weak; const struct bpf_func_proto bpf_get_current_comm_proto __weak; +const struct bpf_func_proto bpf_strnlen __weak; const struct bpf_func_proto * __weak bpf_get_trace_printk_proto(void) { diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c index 3991840..ec64244 100644 --- a/kernel/bpf/helpers.c +++ b/kernel/bpf/helpers.c @@ -120,6 +120,17 @@ const struct bpf_func_proto bpf_get_current_pid_tgid_proto = { .ret_type = RET_INTEGER, }; +BPF_CALL_2(bpf_strnlen, const char __user *, buf, u32, size) +{ + return strnlen_user(buf, size); +} unfortunately it's not so straightforward, since

strnlen_user() can only be called on user pointer in user context. access_ok() macro just recently got WARN_ON_IN_IRQ() warning to prevent such misuse. btw, verifier just recently got support for variable length bpf_probe_read(buf, len, unsafe_ptr) where 'buf' can be a pointer inside map's value and 'len' can be capped variable. I think we need to solve it more generically than adding strlen/strcpy helpers.

…

+ +const struct bpf_func_proto bpf_strnlen_proto = { + .func = bpf_strnlen, + .gpl_only = false, + .ret_type = RET_INTEGER, +}; +

kbukin1 · 2017-01-12T01:07:17Z

strnlen_user() can only be called on user pointer in user context.

Right (that is the reason for "see comments for strnlen_user"). These are the only strings I am interested in. Do you know if that could be jit-enforced? A new function could be renamed into something like bpf_strnlen_user() until a more general solution is available.

4ast · 2017-01-12T01:15:46Z

On Wed, Jan 11, 2017 at 5:07 PM, Konstantin Bukin ***@***.***> wrote: strnlen_user() can only be called on user pointer in user context. Right (that is the reason for "see comments for strnlen_user"). These are the only strings I am interested in. Do you know if that could be jit-enforced? A new function could be renamed into something like bpf_strnlen_user() until a more general solution is available.

we can do something like bpf_probe_write_user() but it would restrict it too much. It's hard to justify adding a helper for single use case.

gianlucaborello · 2017-01-12T01:29:31Z

I am also very interested in a similar use case, I'll provide a couple more points on this.

As @4ast knows, I worked on a strncpy helper that can operate on unsafe memory: gianlucaborello/linux@8239c92

Overall, I also like a strnlen approach (which you can then combine with bpf_probe_read), but done with code borrowed from trace_kprobe.c:

/* Return the length of string -- including null terminal byte */
static void FETCH_FUNC_NAME(memory, string_size)(struct pt_regs *regs,
						 void *addr, void *dest)
{
	mm_segment_t old_fs;
	int ret, len = 0;
	u8 c;

	old_fs = get_fs();
	set_fs(KERNEL_DS);
	pagefault_disable();

	do {
		ret = __copy_from_user_inatomic(&c, (u8 *)addr + len, 1);
		len++;
	} while (c && ret == 0 && len < MAX_STRING_SIZE);

	pagefault_enable();
	set_fs(old_fs);

	if (ret < 0)	/* Failed to check the length */
		*(u32 *)dest = 0;
	else
		*(u32 *)dest = len;
}

I haven't tried this, but it should work (I apologize if it doesn't and I just assumed) and it's safe from an atomic context (unlike strnlen_user()) and also works against any type of memory (which is a huge plus, in my use case I frequently deal with strings from user as well as dereferencing kernel strings).

Also, if you want to work on data bigger than 512 bytes don't use the stack, the verifier error you're getting is legit, but as @4ast says you can now use a map element value (which is what I'm doing in my project). Please find more details here: https://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/commit/?id=5722569bb9c3bd922c4f10b5b2912fe88c255312

gianlucaborello · 2017-01-20T18:28:33Z

@kbukin1 the previously mentioned strncpy helper landed in net-next today: https://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/commit/?id=a5e8c07059d0f0b31737408711d44794928ac218. That, combined with the recent changes of allowing variable memory accesses (https://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/commit/kernel/bpf/verifier.c?id=06c1c049721a995dee2829ad13b24aaf5d7c5cce, so that you can use the string length discovered at runtime as buffer size in other helpers) and the ability to use maps directly as buffer pointers with helpers (https://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/commit/kernel/bpf/verifier.c?id=5722569bb9c3bd922c4f10b5b2912fe88c255312, so that you can bypass the limited stack size) should cover your use case quite well I suppose. I am writing a large BPF program and am able to get a lot of stuff done with these new additions.

Birch-san mentioned this issue Feb 10, 2019

str() to support large strings bpftrace/bpftrace#305

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dealing with long path names #900

Dealing with long path names #900

kbukin1 commented Jan 11, 2017

brendangregg commented Jan 11, 2017

kbukin1 commented Jan 12, 2017

4ast commented Jan 12, 2017 via email

kbukin1 commented Jan 12, 2017

4ast commented Jan 12, 2017 via email

gianlucaborello commented Jan 12, 2017

gianlucaborello commented Jan 20, 2017

Dealing with long path names #900

Dealing with long path names #900

Comments

kbukin1 commented Jan 11, 2017

brendangregg commented Jan 11, 2017

kbukin1 commented Jan 12, 2017

4ast commented Jan 12, 2017 via email

kbukin1 commented Jan 12, 2017

4ast commented Jan 12, 2017 via email

gianlucaborello commented Jan 12, 2017

gianlucaborello commented Jan 20, 2017