Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix BPF multi-uprobe PID filtering logic #7070

Closed
wants to merge 5 commits into from
Closed

Fix BPF multi-uprobe PID filtering logic #7070

wants to merge 5 commits into from

Conversation

kernel-patches-daemon-bpf[bot]
Copy link

Pull request for series with
subject: Fix BPF multi-uprobe PID filtering logic
version: 1
url: https://patchwork.kernel.org/project/netdevbpf/list/?series=854543

@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: 4b377b4
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=854543
version: 1

@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: 4b377b4
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=854781
version: 2

@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: 8d00547
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=854781
version: 2

facebook-github-bot pushed a commit to facebookincubator/kernel-patches-daemon that referenced this pull request May 23, 2024
@facebook-github-bot
Do not use results from previous series version
1dcda88 
Summary:
Previously, we could see the following sequence of events:

```
<v1 sent>
email: v1 fail
<v2 sent>
email: v2 fail       <<<
email: v2 success
```

`<<<` indicates that kpd was looking at the results from v1
but reporting against v2. This is confusing and wrong.

Example: kernel-patches/bpf#7070

 {F1647837371}

## Root cause

kpd, for each series on patchwork, does a reconcile-then-reap action.

In our case, during reconcile, kpd would:

1. Grab a handle to the existing PR
2. Remove v1 labels and add v2 labels
3. Force push the PR

At the end of reconciliation, the handle to the PR would have stale metadata
saying the PR HEAD still pointed to v1 (despite v2 already being pushed).

This means that when kpd goes to reap the results, it would filter workflow
runs based on the v1 sha, see the jobs have completed, and then report
the stale result against v2.

Fix by syncing the handle's metadata after a force push.

Reviewed By: chantra

Differential Revision: D57741395

fbshipit-source-id: 2974327c247b8bfc4028e566ab14254a53c9b891
@anakryiko
bpf: fix multi-uprobe PID filtering logic
1e8b184 
Current implementation of PID filtering logic for multi-uprobes in
uprobe_prog_run() is filtering down to exact *thread*, while the intent
for PID filtering it to filter by *process* instead. The check in
uprobe_prog_run() also differs from the analogous one in
uprobe_multi_link_filter() for some reason. The latter is correct,
checking task->mm, not the task itself.

Fix the check in uprobe_prog_run() to perform the same task->mm check.

While doing this, we also update get_pid_task() use to use PIDTYPE_TGID
type of lookup, given the intent is to get a representative task of an
entire process. This doesn't change behavior, but seems more logical. It
would hold task group leader task now, not any random thread task.

Last but not least, given multi-uprobe support is half-broken due to
this PID filtering logic (depending on whether PID filtering is
important or not), we need to make it easy for user space consumers
(including libbpf) to easily detect whether PID filtering logic was
already fixed.

We do it here by adding an early check on passed pid parameter. If it's
negative (and so has no chance of being a valid PID), we return -EINVAL.
Previous behavior would eventually return -ESRCH ("No process found"),
given there can't be any process with negative PID. This subtle change
won't make any practical change in behavior, but will allow applications
to detect PID filtering fixes easily. Libbpf fixes take advantage of
this in the next patch.

Cc: stable@vger.kernel.org
Acked-by: Jiri Olsa <jolsa@kernel.org>
Fixes: b733eea ("bpf: Add pid filter support for uprobe_multi link")
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
@anakryiko
bpf: remove unnecessary rcu_read_{lock,unlock}() in multi-uprobe atta…
5c538d7 
…ch logic

get_pid_task() internally already calls rcu_read_lock() and
rcu_read_unlock(), so there is no point to do this one extra time.

This is a drive-by improvement and has no correctness implications.

Acked-by: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
@anakryiko
libbpf: detect broken PID filtering logic for multi-uprobe
86d8ff3 
Libbpf is automatically (and transparently to user) detecting
multi-uprobe support in the kernel, and, if supported, uses
multi-uprobes to improve USDT attachment speed.

USDTs can be attached system-wide or for the specific process by PID. In
the latter case, we rely on correct kernel logic of not triggering USDT
for unrelated processes.

As such, on older kernels that do support multi-uprobes, but still have
broken PID filtering logic, we need to fall back to singular uprobes.

Unfortunately, whether user is using PID filtering or not is known at
the attachment time, which happens after relevant BPF programs were
loaded into the kernel. Also unfortunately, we need to make a call
whether to use multi-uprobes or singular uprobe for SEC("usdt") programs
during BPF object load time, at which point we have no information about
possible PID filtering.

The distinction between single and multi-uprobes is small, but important
for the kernel. Multi-uprobes get BPF_TRACE_UPROBE_MULTI attach type,
and kernel internally substitiute different implementation of some of
BPF helpers (e.g., bpf_get_attach_cookie()) depending on whether uprobe
is multi or singular. So, multi-uprobes and singular uprobes cannot be
intermixed.

All the above implies that we have to make an early and conservative
call about the use of multi-uprobes. And so this patch modifies libbpf's
existing feature detector for multi-uprobe support to also check correct
PID filtering. If PID filtering is not yet fixed, we fall back to
singular uprobes for USDTs.

This extension to feature detection is simple thanks to kernel's -EINVAL
addition for pid < 0.

Acked-by: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
@anakryiko
selftests/bpf: extend multi-uprobe tests with child thread case
3cc538d 
Extend existing multi-uprobe tests to test that PID filtering works
correctly. We already have child *process* tests, but we need also child
*thread* tests. This patch adds spawn_thread() helper to start child
thread, wait for it to be ready, and then instruct it to trigger desired
uprobes.

Additionally, we extend BPF-side code to track thread ID, not just
process ID. Also we detect whether extraneous triggerings with
unexpected process IDs happened, and validate that none of that happened
in practice.

These changes prove that fixed PID filtering logic for multi-uprobe
works as expected. These tests fail on old kernels.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
Acked-by: Jiri Olsa <jolsa@kernel.org>
@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: 44382b3
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=854781
version: 2

@anakryiko
selftests/bpf: extend multi-uprobe tests with USDTs
a61a184 
Validate libbpf's USDT-over-multi-uprobe logic by adding USDTs to
existing multi-uprobe tests. This checks correct libbpf fallback to
singular uprobes (when run on older kernels with buggy PID filtering).
We reuse already established child process and child thread testing
infrastructure, so additions are minimal. These test fail on either
older kernels or older version of libbpf that doesn't detect PID
filtering problems.

Acked-by: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
@kernel-patches-daemon-bpf
Copy link
Author

At least one diff in series https://patchwork.kernel.org/project/netdevbpf/list/?series=854781 irrelevant now. Closing PR.

@kernel-patches-daemon-bpf kernel-patches-daemon-bpf bot deleted the series/854543=>bpf branch May 25, 2024 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
accepted bpf V2-ci-pass V2
Projects
None yet
Milestone
No milestone
1 participant
@anakryiko