Add fd_path builtin function #1060

olsajiri · 2020-01-03T15:13:57Z

Adding fd_path builtin function that returns full path
for given file descriptor, syntax:

char *fd_path(int fd);

Example:

  $ cat ex.bt
  tracepoint:syscalls:sys_enter_newfstat
  {
    printf("comm %s, pid %d, fd %d %s\n", comm, pid, args->fd, fd_path(args->fd));
  }

  $ sudo bpftrace ex.bt
  Attaching 1 probe...
  comm NetworkManager, pid 1178, fd 24 /proc/sys/net/ipv6/conf/eno2/disable_ipv6
  comm ls, pid 5878, fd 3 /etc/ld.so.cache
  comm ls, pid 5878, fd 3 /usr/lib/locale/en_US.utf8/LC_MESSAGES/SYS_LC_MESSAGES
  comm ls, pid 5878, fd 1 /dev/pts/0
  comm systemd, pid 1, fd 19 /proc/945/cgroup

The fd_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.

fbs

Hah, this wasn't even merged yesterday (#29 )

Looks ok for a first version. It would be nice to be able to limit the string length in a future to save some stack space

fbs · 2020-01-03T15:34:02Z

docs/reference_guide.md

@@ -2163,6 +2164,24 @@ Attaching 320 probes...
 @[mpv, tracepoint:syscalls:sys_enter_getpid]: 31178
 @[mpv, tracepoint:syscalls:sys_enter_futex]: 403868
 ```
+## 20. `fd_path()`: Returns path of the file refferenced with file descriptor in argument


Suggested change

## 20. `fd_path()`: Returns path of the file refferenced with file descriptor in argument

## 20. `fd_path()`: Returns path of the file referenced by file descriptor in argument

fbs · 2020-01-03T15:35:57Z

tests/CMakeLists.txt

@@ -94,6 +94,9 @@ set_property(
 endif()
 target_compile_definitions(bpftrace_test PRIVATE HAVE_SEND_SIGNAL)

+if(HAVE_GET_FD_PATH)
+  target_compile_definitions(bpftrace_test PRIVATE HAVE_GET_FD_PATH)


For send_signal I've always defined this(unconditionally), and manually defined BPF_FUNC_send_signal to always have the tests work, as we can do semantic and codegen tests without kernel support. You'll will have to do the same thing here.

I see, sounds good

fbs · 2020-01-03T15:36:52Z

tests/semantic_analyser.cpp

+{
+  test("kprobe:f { $k = fd_path( 1 ) }", 0);
+  test("kretprobe:f { $k = fd_path( 1 ) }", 0);
+  test("tracepoint:category:event { $k = fd_path( 1 ) }", 0);


would be good to test some failures too fd_path(-100) fd_path("abc").

And fd_path(arg0) as that is valid too.

fbs · 2020-01-03T15:38:08Z

docs/reference_guide.md

@@ -1534,6 +1534,7 @@ Tracing block I/O sizes > 0 bytes
 - `ustack([StackMode mode, ][int level])` - User stack trace
 - `ntop([int af, ]int|char[4|16] addr)` - Convert IP address data to text
 - `cat(char *filename)` - Print file content
+- `fd_path(int fd)` - Returns path of the file refferenced with file descriptor in argument


same as below

danobi · 2020-01-03T17:38:01Z

All our other 2-word helpers have been _ free (eg kstack, ustack, cgroupid). Would it be better if this was path or fdpath?

olsajiri · 2020-01-03T18:11:05Z

All our other 2-word helpers have been _ free (eg kstack, ustack, cgroupid). Would it be better if this was path or fdpath?

I have another helper coming that gives path from 'struct file',
so I'd prefer the prefix.. 'fdpath' is ok for me.. the other one could
be filepath

olsajiri · 2020-01-03T18:14:39Z

All our other 2-word helpers have been _ free (eg kstack, ustack, cgroupid). Would it be better if this was path or fdpath?

I have another helper coming that gives path from 'struct file',
so I'd prefer the prefix.. 'fdpath' is ok for me.. the other one could
be filepath

or we could make one polymorph path function ;-) we could actually check
on the argument type and call appropriate helper.. will check

brendangregg · 2020-01-03T18:41:22Z

Thanks Jiri,

Good to see BPF_FUNC_get_fd_path get picked up so quick. :)

I'd prefer no underscore, so just "fdpath" and the next one can be "filepath".

I'm not sure about polymorph, I don't think we have anything else in bpftrace like that. How would it work? If it was, say, fpath(), then people might assume it works for everything, and try a dentry with it. The docs would have to say it's for fds and files only. Would that be confusing that it's not anything? Might be ok if it was documented as "fpath(): return a file path from int fd or struct file".

olsajiri · 2020-01-04T16:51:53Z

on the second thought there's not too much gain in this given the confusion it could
cause plus the added complexity in the code.. fdpath and filepath will be better

It's sometimes convenient to use other kernel headers, now it's possible possible with new KERNEL_INCLUDE_DIRS build variable, like: $ cd <kernel-dir> $ make INSTALL_HDR_PATH=/tmp/headers headers_install $ cd <bpftrace-dir> $ cmake -DKERNEL_INCLUDE_DIRS=/tmp/headers/include/ ... Signed-off-by: Jiri Olsa <jolsa@redhat.com>

And based on that define HAVE_GET_FD_PATH macro for c++ code. Signed-off-by: Jiri Olsa <jolsa@redhat.com>

Adding fdpath builtin function that returns full path for given file descriptor, syntax: char *fdpath(int fd); Example: $ cat ex.bt tracepoint:syscalls:sys_enter_newfstat { printf("comm %s, pid %d, fd %d %s\n", comm, pid, args->fd, fdpath(args->fd)); } $ sudo bpftrace ex.bt Attaching 1 probe... comm NetworkManager, pid 1178, fd 24 /proc/sys/net/ipv6/conf/eno2/disable_ipv6 comm ls, pid 5878, fd 3 /etc/ld.so.cache comm ls, pid 5878, fd 3 /usr/lib/locale/en_US.utf8/LC_MESSAGES/SYS_LC_MESSAGES comm ls, pid 5878, fd 1 /dev/pts/0 comm systemd, pid 1, fd 19 /proc/945/cgroup The fdpath 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>

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

fbs · 2020-01-10T08:41:26Z

src/ast/irbuilderbpf.h

@@ -62,6 +62,7 @@ class IRBuilderBPF : public IRBuilder<>
  void        CreateGetCurrentComm(AllocaInst *buf, size_t size);
  void        CreatePerfEventOutput(Value *ctx, Value *data, size_t size);
  void        CreateSignal(Value *sig);
+  void CreateFdPath(AllocaInst *path, Value *fd);


Can you indent this one? I've opened #1074 stop clang-format from complaining about this

fbs

Found some typing issues in the ir. They don't really break llvm but good to fix them anyway

fbs · 2020-01-10T08:45:28Z

src/ast/irbuilderbpf.cpp

+  // int bpf_get_fdpath(char *path, u32 size, int fd)
+  // Return: 0 or error
+  FunctionType *fdpath_func_type = FunctionType::get(
+      getInt64Ty(), { getInt8PtrTy(), getInt32Ty(), getInt32Ty() }, false);


Suggested change

getInt64Ty(), { getInt8PtrTy(), getInt32Ty(), getInt32Ty() }, false);

getInt64Ty(), { path->getType(), getInt32Ty(), getInt32Ty() }, false);

To avoid type issues.

right, thanks

fbs · 2020-01-10T08:46:47Z

src/ast/irbuilderbpf.cpp

+  Constant *fdpath_func = ConstantExpr::getCast(Instruction::IntToPtr,
+                                                getInt64(BPF_FUNC_get_fd_path),
+                                                fdpath_func_ptr_type);
+  CreateCall(fdpath_func, { path, getInt64(bpftrace_.strlen_), fd }, "fdpath");


Suggested change

CreateCall(fdpath_func, { path, getInt64(bpftrace_.strlen_), fd }, "fdpath");

CreateCall(fdpath_func, { path, getInt32(bpftrace_.strlen_), CreateIntCast(fd, getInt32Ty(), false) }, "fdpath");

To match the types used in the function prototype

oops, thanks

fbs · 2020-01-10T08:58:10Z

src/ast/semantic_analyser.cpp

+#ifdef HAVE_GET_FD_PATH
+    if (check_varargs(call, 1, 1))
+    {
+      check_arg(call, Type::integer, 0);


Does fdpath work for all probe types or should we restrict it? E.g i:s:1 { @[pid,fdpath(1)]++ }

I added the check.. it's supported by tracepoints, I'm using kernel patch that adds it for kprobes,which I'll post

however the helper still did not get merged.. moreover got outrun by another one, so the ID changed ;-)) I'll wait with new push until it's merged, to avoid any confusion

brendangregg · 2020-02-11T22:26:24Z

I'd merge this, but get_fd_path() hasn't quite landed in Linux yet. Next release maybe?

olsajiri · 2020-02-12T15:23:19Z

let's hope ;-) the kernel change waits for vfs review, I asked al Viro for review

bpftrace change actually needs the new helper ID hardcoded, so we need to
wait for the kernel in any case, because new helpers are coming all the time

danobi · 2020-11-19T01:27:34Z

#1492 supercedes

fbs reviewed Jan 3, 2020

View reviewed changes

olsajiri added 2 commits January 4, 2020 18:17

Detect BPF_FUNC_get_fd_path support

6867dd1

And based on that define HAVE_GET_FD_PATH macro for c++ code. Signed-off-by: Jiri Olsa <jolsa@redhat.com>

olsajiri force-pushed the fd_path branch from d501cea to 7a909fd Compare January 4, 2020 18:43

olsajiri added 2 commits January 4, 2020 20:42

Add fdpath tests

3dee6e5

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

olsajiri force-pushed the fd_path branch from 7a909fd to 3dee6e5 Compare January 4, 2020 19:43

fbs reviewed Jan 10, 2020

View reviewed changes

fbs mentioned this pull request Jan 17, 2020

Runtime feature checking #1088

Merged

danobi closed this Nov 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add fd_path builtin function #1060

Add fd_path builtin function #1060

olsajiri commented Jan 3, 2020

fbs left a comment

fbs Jan 3, 2020

fbs Jan 3, 2020

olsajiri Jan 3, 2020

fbs Jan 3, 2020

olsajiri Jan 3, 2020

fbs Jan 3, 2020

danobi commented Jan 3, 2020

olsajiri commented Jan 3, 2020 •

edited

Loading

olsajiri commented Jan 3, 2020

brendangregg commented Jan 3, 2020

olsajiri commented Jan 4, 2020

fbs Jan 10, 2020

olsajiri Jan 13, 2020

fbs left a comment

fbs Jan 10, 2020

olsajiri Jan 13, 2020

fbs Jan 10, 2020

olsajiri Jan 13, 2020

fbs Jan 10, 2020

olsajiri Jan 13, 2020

brendangregg commented Feb 11, 2020

olsajiri commented Feb 12, 2020

danobi commented Nov 19, 2020

	## 20. `fd_path()`: Returns path of the file refferenced with file descriptor in argument
	## 20. `fd_path()`: Returns path of the file referenced by file descriptor in argument

	getInt64Ty(), { getInt8PtrTy(), getInt32Ty(), getInt32Ty() }, false);
	getInt64Ty(), { path->getType(), getInt32Ty(), getInt32Ty() }, false);

	CreateCall(fdpath_func, { path, getInt64(bpftrace_.strlen_), fd }, "fdpath");
	CreateCall(fdpath_func, { path, getInt32(bpftrace_.strlen_), CreateIntCast(fd, getInt32Ty(), false) }, "fdpath");

Add fd_path builtin function #1060

Add fd_path builtin function #1060

Conversation

olsajiri commented Jan 3, 2020

fbs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danobi commented Jan 3, 2020

olsajiri commented Jan 3, 2020 • edited Loading

olsajiri commented Jan 3, 2020

brendangregg commented Jan 3, 2020

olsajiri commented Jan 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendangregg commented Feb 11, 2020

olsajiri commented Feb 12, 2020

danobi commented Nov 19, 2020

olsajiri commented Jan 3, 2020 •

edited

Loading