Add path builtin function #1492

olsajiri · 2020-08-30T22:20:00Z

Adding dpath builtin function that returns full path
referenced by struct path pointer in argument.

dpath(struct path *path)

Example:

  # bpftrace  -e 'kfunc:filp_close { printf("%s\n", dpath(args->filp->f_path)); }'
  Attaching 1 probe...
  /proc/sys/net/ipv6/conf/eno2/disable_ipv6
  /proc/sys/net/ipv6/conf/eno2/use_tempaddr
  socket:[23276]
  /proc/sys/net/ipv6/conf/eno2/disable_ipv6
  socket:[17655]
  /sys/devices/pci0000:00/0000:00:1c.5/0000:04:00.1/net/eno2/type
  socket:[38745]
  /proc/sys/net/ipv6/conf/eno2/disable_ipv6

The dpath returns the string with the path or empty string
on error. I don't think it's necessary to return error code
at the moment, however it can be added later in the second
optional argument.

fbs · 2020-08-31T10:38:19Z

src/bpffeature.cpp

+  if (has_d_path_.has_value())
+    return *has_d_path_;
+
+  struct bpf_insn insns[] = {


Why does this need a custom program? doesn't the helper detection work here?

aah, I try to load the working program and check that try_load succeeds,
but I did not noticed that the detection stuff checks for 'invalid' string,
so the load can fail for other reasons but still detect the helper.. that might
actually work.. I'll try it ;-)

thanks

hm, the tracing program fails earlier with:
Tracing programs must provide btf_id

so we need to have special function for that

fbs · 2020-08-31T10:38:58Z

Nice! Glad this helper finally landed

danobi · 2020-09-01T16:51:09Z

Couple things:

are we locked into "dpath" name? I feel like "path" makes more sense here
last I heard bpf_d_path() works for whitelisted functions in the kernel. is that still the case? if so, I think it'd be important to return error code

wilsonwr · 2020-09-01T17:15:56Z

are we locked into "dpath" name? I feel like "path" makes more sense here

Personally, I appreciated it being called "dpath" when I saw this pull request. It helped me quickly understand that this function is like d_path() of the Linux kernel's filesystem API. Just my two cents.

danobi · 2020-09-01T23:17:24Z

are we locked into "dpath" name? I feel like "path" makes more sense here

Personally, I appreciated it being called "dpath" when I saw this pull request. It helped me quickly understand that this function is like d_path() of the Linux kernel's filesystem API. Just my two cents.

Yeah, I buy that. @olsajiri also made a good point in the IRC that fd_path() might still come one day.

brendangregg · 2020-09-01T23:54:55Z

If possible, we shouldn't wed functions to Linux internals. There's already interest to see bpftrace on BSD and other OSes.

For example, I think an fdpath() function makes sense across kernels, since they all have a notion of an FD (POSIX).

This is trickier since it's for Linux struct dentry. I guess options are:

A) We go with dpath() and the docs say "this is a Linux specific function." BSD might add a vpath() to its port, etc.

B) We go with path() and the docs say "On Linux, this turns a dentry to a path; on BSD, this tuns a vnode to a path." etc.

C) The docs say "dpath() is an internal function that you should not use directly," and then we add a fdpath() wrapper that maps to code for doing the FD to dentry and then dpath() call (I'm sure I have examples of FD->dentry in the BPF book). Then we document fdpath() and it should work once other OSes have their own implementation.

D) We turn this PR into fdpath() with the FD->dentry code built in. :)

(C/D) Solves the problem of forking scripts and documentation over something trivial: path lookups, which will be used by a bunch of one-liners. It would be nice if basic scripts/documentation didn't fork so quickly (it will be necessary for deeper things of course).

Just thinking of a one-liner one might see in a hello-world-level tutorial:

# bpftrace -e 't:syscalls:sys_enter_read { @paths[fdpath(args->fd)] = count(); }'

(probe types [t for tracepoint] and their arguments are of course currently Linux-influenced; although if other OSes just use the abbreviations it mostly works: k: for kernel dynamic instrumentation, u: for user-level dynamic instrumentation, U: for user-level static instrumentation, and t: for...hang on, perhaps we should add a K: alternate alias for for kernel static instrumentation, and then we have an OS-agnostic set!).

@olsajiri what ended up in the whitelist? syscall tracepoints, ...?

olsajiri · 2020-09-02T14:58:35Z

If possible, we shouldn't wed functions to Linux internals. There's already interest to see bpftrace on BSD and other OSes.

For example, I think an fdpath() function makes sense across kernels, since they all have a notion of an FD (POSIX).

This is trickier since it's for Linux struct dentry. I guess options are:

A) We go with dpath() and the docs say "this is a Linux specific function." BSD might add a vpath() to its port, etc.

B) We go with path() and the docs say "On Linux, this turns a dentry to a path; on BSD, this tuns a vnode to a path." etc.

hm, for the sake of scripts I think maybe we should use just 'path'

we will overload path's argument to be different type,
but it'd be for different systems, so they would not collide
at the same time

C) The docs say "dpath() is an internal function that you should not use directly," and then we add a fdpath() wrapper that maps to code for doing the FD to dentry and then dpath() call (I'm sure I have examples of FD->dentry in the BPF book). Then we document fdpath() and it should work once other OSes have their own implementation.

D) We turn this PR into fdpath() with the FD->dentry code built in. :)

I might have confused things when mentioning dentry.. it was the answer for
where's the 'd' in d_path coming from.. but the dpath function argument
is actually 'struct path' object, which is also argument for kernel's d_path
function

(C/D) Solves the problem of forking scripts and documentation over something trivial: path lookups, which will be used by a bunch of one-liners. It would be nice if basic scripts/documentation didn't fork so quickly (it will be necessary for deeper things of course).

Just thinking of a one-liner one might see in a hello-world-level tutorial:
# bpftrace -e 't:syscalls:sys_enter_read { @paths[fdpath(args->fd)] = count(); }'
(probe types [t for tracepoint] and their arguments are of course currently Linux-influenced; although if other OSes just use the abbreviations it mostly works: k: for kernel dynamic instrumentation, u: for user-level dynamic instrumentation, U: for user-level static instrumentation, and t: for...hang on, perhaps we should add a K: alternate alias for for kernel static instrumentation, and then we have an OS-agnostic set!).

@olsajiri what ended up in the whitelist? syscall tracepoints, ...?

vfs_truncate
vfs_fallocate
dentry_open
vfs_getattr
filp_close

not much ;-) I recall we discussed that at some point you could provide list
of functions you need on whitelist and fs folks would review if that's ok

the kernel change is just matter of adding line like:

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index b2a5380eb187..4cff2f891d28 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1123,6 +1123,7 @@ BTF_ID(func, vfs_fallocate)
 BTF_ID(func, dentry_open)
 BTF_ID(func, vfs_getattr)
 BTF_ID(func, filp_close)
+BTF_ID(func, function)
 BTF_SET_END(btf_allowlist_d_path)
 
 static bool bpf_d_path_allowed(const struct bpf_prog *prog)

I think we could add support for tracepoints as well,
but I'd like to add raw tracepoint support first to bpftrace,
which is tricky ATM and needs some pahole changes

javierhonduco · 2020-09-08T10:01:31Z

This is very exciting! 😄 cc @tyroguru

olsajiri · 2020-09-25T13:28:33Z

The latest version renames 'dpath' to 'path', could you please check? thanks @fbs @danobi @brendangregg

fbs · 2020-09-25T13:46:58Z

Looks ok 👍

Can you add a runtime test for this? By doing something like 5441336 you can limit the tests to systems that support it:

NAME openat uptr
RUN bpftrace -v -e 't:syscalls:sys_enter_openat /comm == "syscall"/ { print(str(uptr(args->filename))) }' -c "./testprogs/syscall openat"
EXPECT bpftrace_runtime_test_syscall
REQUIRES_FEATURE probe_read_kernel
TIMEOUT 5

olsajiri · 2020-09-25T15:34:49Z

Looks ok +1

Can you add a runtime test for this? By doing something like 5441336 you can limit the tests to systems that support it:
NAME openat uptr
RUN bpftrace -v -e 't:syscalls:sys_enter_openat /comm == "syscall"/ { print(str(uptr(args->filename))) }' -c "./testprogs/syscall openat"
EXPECT bpftrace_runtime_test_syscall
REQUIRES_FEATURE probe_read_kernel
TIMEOUT 5

ah, did not see REQUIRES_FEATURE, nice.. will add

fbs · 2020-10-02T08:54:14Z

@brendangregg @mmisono @danobi . Code looks ok (but I don't have a good btf test system atm). Any thoughts on this one?

mmisono · 2020-10-02T12:06:35Z

BPF_FUNC_d_path is not merged in mainline yet, is it?

bpftrace_test does not run w/o root.

/home/ubuntu/work/bpftrace/bpftrace/tests/semantic_analyser.cpp:70: Failure
Expected equality of these values:
  fields.analyse()
    Which is: 1
  expected_field_analyser
    Which is: 0

Input:
kfunc:filp_close { $k = path( args->filp->f_path ) }

Output:
stdin:1:1-17: ERROR: kfunc:filp_close: could not read traceable functions from /sys/kernel/debug/tracing/available_filter_functions (is debugfs mounted?)
kfunc:filp_close { $k = path( args->filp->f_path ) }
~~~~~~~~~~~~~~~~

/home/ubuntu/work/bpftrace/bpftrace/tests/semantic_analyser.cpp:70: Failure
Expected equality of these values:
  fields.analyse()
    Which is: 1
  expected_field_analyser
    Which is: 0

Input:
kretfunc:fget { $k = path( retval->f_path ) }

Output:
stdin:1:1-14: ERROR: kfunc:fget: could not read traceable functions from /sys/kernel/debug/tracing/available_filter_functions (is debugfs mounted?)
kretfunc:fget { $k = path( retval->f_path ) }
~~~~~~~~~~~~~

[  FAILED  ] semantic_analyser.call_path (788 ms)

mmisono · 2020-10-02T12:24:45Z

@olsajiri
Could you include the error checking code? mmisono@bddb3c2
(I assume that dpath return <0 if error)

olsajiri · 2020-10-02T20:28:06Z

@olsajiri
Could you include the error checking code? mmisono@bddb3c2
(I assume that dpath return <0 if error)

sure, will fix that, thnx

fbs · 2020-10-04T11:42:07Z

tests/semantic_analyser.cpp

@@ -791,6 +791,24 @@ TEST(semantic_analyser, call_stack)
  test("kprobe:f { @x = 3; ustack(perf, @x) }", 10);
 }

+TEST(semantic_analyser, call_path)
+{
+  BPFfeature feature;


Suggested change

BPFfeature feature;

MockBPFFeature handles this part. Allows us to run tests without root.

olsajiri · 2020-10-04T21:09:43Z

@olsajiri
Could you include the error checking code? mmisono@bddb3c2
(I assume that dpath return <0 if error)

sure, will fix that, thnx

I checked and we don't check for error in there, it returns void,
but I should have used createCall call instead

mmisono · 2020-10-05T01:21:20Z

@olsajiri
Could you include the error checking code? mmisono@bddb3c2
(I assume that dpath return <0 if error)

sure, will fix that, thnx

I checked and we don't check for error in there, it returns void,
but I should have used createCall call instead

The commit is for reporting helper errors if any (the functionality introduced in #1276.) Or do you mean bpf_d_path() helper returns void?

olsajiri · 2020-10-05T08:41:45Z

@olsajiri
Could you include the error checking code? mmisono@bddb3c2
(I assume that dpath return <0 if error)

sure, will fix that, thnx

I checked and we don't check for error in there, it returns void,
but I should have used createCall call instead

The commit is for reporting helper errors if any (the functionality introduced in #1276.) Or do you mean bpf_d_path() helper returns void?

ah I misunderstood, sry.. yes, it's good change, I'll add it, thanks

tests/semantic_analyser.cpp

danobi · 2020-10-08T23:21:49Z

docs/reference_guide.md

+## 23. `path()`: Returns full path referenced by struct path pointer in argument
+
+Syntax: `path(struct path *path)`
+# bpftrace  -e 'kfunc:filp_close { printf("%s\n", path(args->filp->f_path)); }'
+Attaching 1 probe...
+/proc/sys/net/ipv6/conf/eno2/disable_ipv6
+/proc/sys/net/ipv6/conf/eno2/use_tempaddr
+socket:[23276]
+/proc/sys/net/ipv6/conf/eno2/disable_ipv6
+socket:[17655]
+/sys/devices/pci0000:00/0000:00:1c.5/0000:04:00.1/net/eno2/type
+socket:[38745]
+/proc/sys/net/ipv6/conf/eno2/disable_ipv6
+
+# bpftrace  -e 'kretfunc:dentry_open { printf("%s\n", path(retval->f_path)); }'
+Attaching 1 probe...
+/dev/pts/1 -> /dev/pts/1


This ends up in the example codeblock of sizeof(). Also this should probably go at the end as number 25 (instead of reusing number 23)

right, I reformatted it, thanks

fbs · 2020-10-09T14:11:18Z

Can you rebase this on master? The addrspace stuff just landed.

danobi · 2020-10-09T18:07:03Z

docs/reference_guide.md

+Syntax:
+- `path(struct path *path)`
+
+Return full path referenced by struct path pointer in argument.


One more thing: maybe it'd be good to document there's a whilelist of kernel functions you can use this on. Could be confusing for users who get load errors when they have a correct handle to struct path.

right, I added a sentence about that.. I think we need some way to get the whitelist info from kernel, I'll try to add something

When processing pointer attach point arguments we also need to add pointer record to the BTF set, se we are able to dereference it later on. This will allow to resolve f_path member in dpath call (added in following patches): # bpftrace -e 'kfunc:filp_close { printf("%s\n", dpath(args->filp->f_path)); }' Signed-off-by: Jiri Olsa <jolsa@redhat.com>

Adding fail argument to check_arg so we can have it failed, but still give a chance for other type. It will be used in following patch for d_path check, that allows both pointer and record types in the argument. Signed-off-by: Jiri Olsa <jolsa@redhat.com>

Adding BPFfeature::has_d_path function to test if dpath helper is available. Display it for --info option: # bpftrace --info ... Kernel helpers ... get_current_cgroup_id: yes send_signal: yes override_return: no dpath: yes Signed-off-by: Jiri Olsa <jolsa@redhat.com>

Adding path builtin function that returns full path referenced by struct path pointer in argument. path(struct path *path) Example: # bpftrace -e 'kfunc:filp_close { printf("%s\n", path(args->filp->f_path)); }' Attaching 1 probe... /proc/sys/net/ipv6/conf/eno2/disable_ipv6 /proc/sys/net/ipv6/conf/eno2/use_tempaddr socket:[23276] /proc/sys/net/ipv6/conf/eno2/disable_ipv6 socket:[17655] /sys/devices/pci0000:00/0000:00:1c.5/0000:04:00.1/net/eno2/type socket:[38745] /proc/sys/net/ipv6/conf/eno2/disable_ipv6 The path returns the string with the path or empty string on error. I don't think it's necessary to return error code at the moment, however it can be added later in the second optional argument. Signed-off-by: Jiri Olsa <jolsa@redhat.com>

Signed-off-by: Jiri Olsa <jolsa@redhat.com>

Adding semantic_analyser test. Signed-off-by: Jiri Olsa <jolsa@redhat.com>

Adding path runtime test that checks on close being called on file within './testprogs/syscall read' test prog. Signed-off-by: Jiri Olsa <jolsa@redhat.com>

Signed-off-by: Masanori Misono <m.misono760@gmail.com>

fbs reviewed Aug 31, 2020

View reviewed changes

olsajiri force-pushed the dpath branch from 7014bfc to 101e4d4 Compare September 16, 2020 20:51

olsajiri force-pushed the dpath branch from 101e4d4 to 24abe4b Compare September 24, 2020 21:07

olsajiri changed the title ~~Add dpath builtin function~~ Add path builtin function Sep 26, 2020

olsajiri requested a review from fbs September 29, 2020 07:14

fbs approved these changes Sep 29, 2020

View reviewed changes

olsajiri requested a review from danobi September 30, 2020 11:25

fbs reviewed Oct 4, 2020

View reviewed changes

olsajiri force-pushed the dpath branch from 1dd42a2 to 5ac0cd3 Compare October 4, 2020 21:32

olsajiri force-pushed the dpath branch from 5ac0cd3 to 07c13aa Compare October 5, 2020 09:17

mmisono reviewed Oct 5, 2020

View reviewed changes

tests/semantic_analyser.cpp Show resolved Hide resolved

olsajiri force-pushed the dpath branch from 07c13aa to 79a1249 Compare October 5, 2020 16:21

danobi approved these changes Oct 8, 2020

View reviewed changes

olsajiri force-pushed the dpath branch from 79a1249 to fa967a2 Compare October 9, 2020 09:05

danobi reviewed Oct 9, 2020

View reviewed changes

olsajiri force-pushed the dpath branch from fa967a2 to 8e55ccc Compare October 9, 2020 21:29

olsajiri and others added 8 commits October 12, 2020 11:20

Add fail argument to check_arg

3dd63d4

Adding fail argument to check_arg so we can have it failed, but still give a chance for other type. It will be used in following patch for d_path check, that allows both pointer and record types in the argument. Signed-off-by: Jiri Olsa <jolsa@redhat.com>

Add changelog for path builtin

2948244

Signed-off-by: Jiri Olsa <jolsa@redhat.com>

Add dpath semantic_analyser test

c0d03d2

Adding semantic_analyser test. Signed-off-by: Jiri Olsa <jolsa@redhat.com>

Adding path runtime test

8f11951

Adding path runtime test that checks on close being called on file within './testprogs/syscall read' test prog. Signed-off-by: Jiri Olsa <jolsa@redhat.com>

Check a return value of dpath helper

9ee6139

Signed-off-by: Masanori Misono <m.misono760@gmail.com>

olsajiri force-pushed the dpath branch from 8e55ccc to 9ee6139 Compare October 12, 2020 09:27

fbs merged commit 688cab9 into bpftrace:master Oct 14, 2020

danobi mentioned this pull request Nov 19, 2020

Add fd_path builtin function #1060

Closed

ajor mentioned this pull request Oct 3, 2023

add path() call #29

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add path builtin function #1492

Add path builtin function #1492

olsajiri commented Aug 30, 2020

fbs Aug 31, 2020

olsajiri Sep 1, 2020

olsajiri Sep 16, 2020

fbs commented Aug 31, 2020

danobi commented Sep 1, 2020

wilsonwr commented Sep 1, 2020

danobi commented Sep 1, 2020

brendangregg commented Sep 1, 2020

olsajiri commented Sep 2, 2020

javierhonduco commented Sep 8, 2020

olsajiri commented Sep 25, 2020

fbs commented Sep 25, 2020 •

edited

olsajiri commented Sep 25, 2020

fbs commented Oct 2, 2020

mmisono commented Oct 2, 2020 •

edited

mmisono commented Oct 2, 2020 •

edited

olsajiri commented Oct 2, 2020

fbs Oct 4, 2020

olsajiri commented Oct 4, 2020

mmisono commented Oct 5, 2020

olsajiri commented Oct 5, 2020

danobi Oct 8, 2020

olsajiri Oct 9, 2020

fbs commented Oct 9, 2020

danobi Oct 9, 2020

olsajiri Oct 12, 2020

Add path builtin function #1492

Add path builtin function #1492

Conversation

olsajiri commented Aug 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbs commented Aug 31, 2020

danobi commented Sep 1, 2020

wilsonwr commented Sep 1, 2020

danobi commented Sep 1, 2020

brendangregg commented Sep 1, 2020

olsajiri commented Sep 2, 2020

javierhonduco commented Sep 8, 2020

olsajiri commented Sep 25, 2020

fbs commented Sep 25, 2020 • edited

olsajiri commented Sep 25, 2020

fbs commented Oct 2, 2020

mmisono commented Oct 2, 2020 • edited

mmisono commented Oct 2, 2020 • edited

olsajiri commented Oct 2, 2020

Choose a reason for hiding this comment

olsajiri commented Oct 4, 2020

mmisono commented Oct 5, 2020

olsajiri commented Oct 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbs commented Oct 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbs commented Sep 25, 2020 •

edited

mmisono commented Oct 2, 2020 •

edited

mmisono commented Oct 2, 2020 •

edited