[uprobe] support for pid targeting for shared libs #2830

jordalgo · 2023-11-17T00:06:28Z

This adds support for specifying the pid even when targeting a uprobe/uretprobe in a library shared by multiple pids.

Example:

sudo bpftrace -p 1899508 -e 'uprobe:libc:getaddrinfo {
  print((comm, pid));
}

Issue 2817

Checklist

Language changes are updated in man/adoc/bpftrace.adoc and if needed in docs/reference_guide.md
User-visible and non-trivial changes updated in CHANGELOG.md
The new behaviour is covered by tests

jordalgo · 2023-11-17T00:22:13Z

Note: I looked into making a runtime test for this but it seems pretty tricky in that you'd have to run two programs that use the same shared library and assert you weren't getting samples for one of them.

ajor · 2023-11-17T09:44:23Z

I'm a bit conflicted about this feature. I can definitely see it being useful to retrofit PID-filtering into existing scripts, but I feel like the better way of doing this would be to have the script include a PID predicate itself:

uprobe:libc:getaddrinfo
/ $# == 0 || pid == $1 /
{
  print((comm, pid));
}

If we sort out script-level argument parsing properly then this could be made more powerful by filtering on multiple PIDs, e.g. --pid 123 --pid 124 or --pids 123,124 and / @cli_pids[pid] == 1 /.

ajor · 2023-11-17T12:32:55Z

Seems reasonable to put in at least until we do have proper argument parsing support though.

This new feature applies to all uprobes, not just on shared libs, right?
A runtime test could be done as a program which takes a command line argument, and which is invoked multiple times with different arguments. If we trace this program and print the argument then we can check we've attached to the right PID.

jordalgo · 2023-11-17T14:18:17Z

If we sort out script-level argument parsing properly then this could be made more powerful by filtering on multiple PIDs

It's definitely more powerful doing it this way but I think this change is (at least for me) about matching expected behavior. Because we already support pid filtering for USDTs and uprobes/uretprobes (when there is a wildcard involved) I feel like it makes sense to do this as well for all uprobe/uretprobe invocations.

This new feature applies to all uprobes, not just on shared libs, right?

Yeah I believe so.

A runtime test could be done as a program which takes a command line argument, and which is invoked multiple times with different arguments. If we trace this program and print the argument then we can check we've attached to the right PID.

Maybe I'm confused but I think the problem is not so much attaching to the right pid but testing that we're not attaching to the other pids (or maybe you're saying I should write a NOT_EXPECT predicate). I could write a simple test that passing a pid argument doesn't affect attaching, which ensures we're not breaking existing behavior, but is not testing the new behavior. Maybe an example would help my brain 🧠

ajor · 2023-11-17T15:09:30Z

If we sort out script-level argument parsing properly then this could be made more powerful by filtering on multiple PIDs

It's definitely more powerful doing it this way but I think this change is (at least for me) about matching expected behavior. Because we already support pid filtering for USDTs and uprobes/uretprobes (when there is a wildcard involved) I feel like it makes sense to do this as well for all uprobe/uretprobe invocations.

Yep, makes sense.

For the tests, I was imagining something like this:

BEFORE ./a.out 123
BEFORE ./a.out 456 # SOMEHOW PICK THIS ONE
BEFORE ./a.out 789
RUN bpftrace -p $PID -e 'BEGIN { print("BEGIN") } uprobe:a.out:func { print(arg0) }' END { print("END") }'
EXPECT "BEGIN\n456\nEND"
TIMEOUT 5

Although I don't know how well the test framework will cope with this, particularly getting the PID of a process.

jordalgo · 2023-11-17T22:38:09Z

@ajor Thanks for the suggestion on the test. I got around the limitation of having multiple BEFOREs but needing the BEFORE_PID by just spinning up a program that forks. I confirmed that this test fails on master.

tests/testprogs/uprobe_fork_loop.c

viktormalik

If we sort out script-level argument parsing properly then this could be made more powerful by filtering on multiple PIDs

It's definitely more powerful doing it this way but I think this change is (at least for me) about matching expected behavior. Because we already support pid filtering for USDTs and uprobes/uretprobes (when there is a wildcard involved) I feel like it makes sense to do this as well for all uprobe/uretprobe invocations.

Agreed with @jordalgo here, this is the expected behaviour to me. I don't see a situation where I would want to attach binary uprobes to a PID but attach library uprobes system-wide.

Just a couple of remarks, otherwise looks good, thanks!

viktormalik · 2023-11-20T07:37:27Z

src/attached_probe.cpp

@@ -191,7 +191,7 @@ AttachedProbe::AttachedProbe(Probe &probe,
      // If BPF_PROG_TYPE_RAW_TRACEPOINT is available, no need to attach prog
      // to anything -- we will simply BPF_PROG_RUN it
      if (!feature.has_raw_tp_special())
-        attach_uprobe(safe_mode);
+        attach_uprobe(0, safe_mode);


I'm not sure that this is correct. IIUC, we use process' PID in this case, set in bpftrace.cpp:1090. This should probably use the same approach.

src/attached_probe.cpp

tests/runtime/scripts/uprobe_pid_check.bt

tests/testprogs/uprobe_fork_loop.c

ajor · 2023-11-20T08:09:10Z

tests/testprogs/uprobe_fork_loop.c

+  if (fork() == 0)
+  {
+    spin();
+  }
+  else
+  {
+    spin();
+  }


spin() is run in both the true and false cases - can the if-statement be removed entirely?

Good call. It can. This is what I get from just copying from the interwebs

tests/testprogs/uprobe_fork_loop.c

jordalgo · 2023-11-20T23:37:31Z

Updated based on comments.

viktormalik

LGTM, just two nits (see the CodeQL warnings).

tests/testprogs/uprobe_fork_loop.c

jordalgo · 2023-11-21T14:45:46Z

Lint fixes.

danobi · 2023-11-23T20:38:23Z

src/attached_probe.h

@@ -30,7 +30,8 @@ class AttachedProbe
                BpfProgram &&prog,
                int pid,
                BPFfeature &feature,
-                BTF &btf);
+                BTF &btf,
+                bool safe_mode = false);


Why default to non safe mode?

This default isn't being used by anything checking safe_mode so fine to make it default to true.

danobi · 2023-11-23T20:40:35Z

tests/testprogs/uprobe_fork_loop.c

+
+int main(int argc __attribute__((unused)), char **argv __attribute__((unused)))
+{
+  pid_t p = fork();


Should we wait for the child? Otherwise zombie process right

We want them to be running at the same time.

Ah yeah. Thought spin() had finite iterations. If we rely on runtime test runner to kill the process group this should be fine

jordalgo · 2023-11-24T15:56:59Z

make safe_mode default to true

This adds support for specifying the pid even when targeting a uprobe/uretprobe in a library shared by multiple pids. Example: ``` sudo bpftrace -p 1899508 -e 'uprobe:libc:getaddrinfo { print((comm, pid)); } ``` [Issue 2817](bpftrace#2817)

jordalgo requested review from ajor, viktormalik, danobi and fbs as code owners November 17, 2023 00:06

jordalgo mentioned this pull request Nov 17, 2023

Limit uprobes in shared libraries to a single process when -p is given #2817

Closed

jordalgo force-pushed the uprobe-pid-targeting branch from 206a5e4 to 3be88e5 Compare November 17, 2023 22:35

github-advanced-security bot found potential problems Nov 17, 2023

View reviewed changes

tests/testprogs/uprobe_fork_loop.c Dismissed Show dismissed Hide dismissed

jordalgo force-pushed the uprobe-pid-targeting branch from 3be88e5 to c156b30 Compare November 19, 2023 02:42

viktormalik reviewed Nov 20, 2023

View reviewed changes

ajor reviewed Nov 20, 2023

View reviewed changes

jordalgo force-pushed the uprobe-pid-targeting branch from c156b30 to 6eb2184 Compare November 20, 2023 14:36

github-advanced-security bot found potential problems Nov 20, 2023

View reviewed changes

tests/testprogs/uprobe_fork_loop.c Fixed Show resolved Hide resolved

tests/testprogs/uprobe_fork_loop.c Fixed Show resolved Hide resolved

viktormalik approved these changes Nov 21, 2023

View reviewed changes

tests/testprogs/uprobe_fork_loop.c Fixed Show resolved Hide resolved

tests/testprogs/uprobe_fork_loop.c Fixed Show resolved Hide resolved

jordalgo force-pushed the uprobe-pid-targeting branch from 6eb2184 to 30c49e7 Compare November 21, 2023 13:52

danobi reviewed Nov 23, 2023

View reviewed changes

jordalgo force-pushed the uprobe-pid-targeting branch from 30c49e7 to 3cf96ca Compare November 24, 2023 15:55

danobi approved these changes Nov 24, 2023

View reviewed changes

jordalgo force-pushed the uprobe-pid-targeting branch from 3cf96ca to c83afea Compare November 25, 2023 01:14

danobi merged commit 06aa4aa into bpftrace:master Nov 29, 2023
18 checks passed

jordalgo deleted the uprobe-pid-targeting branch December 18, 2023 15:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[uprobe] support for pid targeting for shared libs #2830

[uprobe] support for pid targeting for shared libs #2830

jordalgo commented Nov 17, 2023 •

edited

Loading

jordalgo commented Nov 17, 2023

ajor commented Nov 17, 2023

ajor commented Nov 17, 2023

jordalgo commented Nov 17, 2023

ajor commented Nov 17, 2023 •

edited

Loading

jordalgo commented Nov 17, 2023

viktormalik left a comment

viktormalik Nov 20, 2023

ajor Nov 20, 2023

jordalgo Nov 20, 2023

jordalgo commented Nov 20, 2023

viktormalik left a comment

jordalgo commented Nov 21, 2023

danobi Nov 23, 2023

jordalgo Nov 23, 2023

danobi Nov 23, 2023

jordalgo Nov 23, 2023

danobi Nov 24, 2023

jordalgo commented Nov 24, 2023

[uprobe] support for pid targeting for shared libs #2830

[uprobe] support for pid targeting for shared libs #2830

Conversation

jordalgo commented Nov 17, 2023 • edited Loading

Checklist

jordalgo commented Nov 17, 2023

ajor commented Nov 17, 2023

ajor commented Nov 17, 2023

jordalgo commented Nov 17, 2023

ajor commented Nov 17, 2023 • edited Loading

jordalgo commented Nov 17, 2023

viktormalik 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

jordalgo commented Nov 20, 2023

viktormalik left a comment

Choose a reason for hiding this comment

jordalgo commented Nov 21, 2023

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

jordalgo commented Nov 24, 2023

jordalgo commented Nov 17, 2023 •

edited

Loading

ajor commented Nov 17, 2023 •

edited

Loading