Skip to content

Commit

Permalink
Merge branch 'Open-coded task_vma iter'
Browse files Browse the repository at this point in the history
Dave Marchevsky says:

====================
At Meta we have a profiling daemon which periodically collects
information on many hosts. This collection usually involves grabbing
stacks (user and kernel) using perf_event BPF progs and later symbolicating
them. For user stacks we try to use BPF_F_USER_BUILD_ID and rely on
remote symbolication, but BPF_F_USER_BUILD_ID doesn't always succeed. In
those cases we must fall back to digging around in /proc/PID/maps to map
virtual address to (binary, offset). The /proc/PID/maps digging does not
occur synchronously with stack collection, so the process might already
be gone, in which case it won't have /proc/PID/maps and we will fail to
symbolicate.

This 'exited process problem' doesn't occur very often as
most of the prod services we care to profile are long-lived daemons, but
there are enough usecases to warrant a workaround: a BPF program which
can be optionally loaded at data collection time and essentially walks
/proc/PID/maps. Currently this is done by walking the vma list:

  struct vm_area_struct* mmap = BPF_CORE_READ(mm, mmap);
  mmap_next = BPF_CORE_READ(rmap, vm_next); /* in a loop */

Since commit 763ecb0 ("mm: remove the vma linked list") there's no
longer a vma linked list to walk. Walking the vma maple tree is not as
simple as hopping struct vm_area_struct->vm_next. Luckily,
commit f39af05 ("mm: add VMA iterator"), another commit in that series,
added struct vma_iterator and for_each_vma macro for easy vma iteration. If
similar functionality was exposed to BPF programs, it would be perfect for our
usecase.

This series adds such functionality, specifically a BPF equivalent of
for_each_vma using the open-coded iterator style.

Notes:
  * This approach was chosen after discussion on a previous series [0] which
    attempted to solve the same problem by adding a BPF_F_VMA_NEXT flag to
    bpf_find_vma.
  * Unlike the task_vma bpf_iter, the open-coded iterator kfuncs here do not
    drop the vma read lock between iterations. See Alexei's response in [0].
  * The [vsyscall] page isn't really part of task->mm's vmas, but
    /proc/PID/maps returns information about it anyways. The vma iter added
    here does not do the same. See comment on selftest in patch 3.
  * bpf_iter_task_vma allocates a _data struct which contains - among other
    things - struct vma_iterator, using BPF allocator and keeps a pointer to
    the bpf_iter_task_vma_data. This is done in order to prevent changes to
    struct ma_state - which is wrapped by struct vma_iterator - from
    necessitating changes to uapi struct bpf_iter_task_vma.

Changelog:

v6 -> v7: https://lore.kernel.org/bpf/[email protected]/

Patch numbers correspond to their position in v6

Patch 2 ("selftests/bpf: Rename bpf_iter_task_vma.c to bpf_iter_task_vmas.c")
  * Add Andrii ack
Patch 3 ("bpf: Introduce task_vma open-coded iterator kfuncs")
  * Add Andrii ack
  * Add missing __diag_ignore_all for -Wmissing-prototypes (Song)
Patch 4 ("selftests/bpf: Add tests for open-coded task_vma iter")
  * Remove two unnecessary header includes (Andrii)
  * Remove extraneous !vmas_seen check (Andrii)
New Patch ("bpf: Add BPF_KFUNC_{START,END}_defs macros")
  * After talking to Andrii, this is an attempt to clean up __diag_ignore_all
    spam everywhere kfuncs are defined. If nontrivial changes are needed,
    let's apply the other 4 and I'll respin as a standalone patch.

v5 -> v6: https://lore.kernel.org/bpf/[email protected]/

Patch 4 ("selftests/bpf: Add tests for open-coded task_vma iter")
  * Remove extraneous blank line. I did this manually to the .patch file
    for v5, which caused BPF CI to complain about failing to apply the
    series

v4 -> v5: https://lore.kernel.org/bpf/[email protected]/

Patch numbers correspond to their position in v4

New Patch ("selftests/bpf: Rename bpf_iter_task_vma.c to bpf_iter_task_vmas.c")
  * Patch 2's renaming of this selftest, and associated changes in the
    userspace runner, are split out into this separate commit (Andrii)

Patch 2 ("bpf: Introduce task_vma open-coded iterator kfuncs")
  * Remove bpf_iter_task_vma kfuncs from libbpf's bpf_helpers.h, they'll be
    added to selftests' bpf_experimental.h in selftests patch below (Andrii)
  * Split bpf_iter_task_vma.c renaming into separate commit (Andrii)

Patch 3 ("selftests/bpf: Add tests for open-coded task_vma iter")
  * Add bpf_iter_task_vma kfuncs to bpf_experimental.h (Andrii)
  * Remove '?' from prog SEC, open_and_load the skel in one operation (Andrii)
  * Ensure that fclose() always happens in test runner (Andrii)
  * Use global var w/ 1000 (vm_start, vm_end) structs instead of two
    MAP_TYPE_ARRAY's w/ 1k u64s each (Andrii)

v3 -> v4: https://lore.kernel.org/bpf/[email protected]/

Patch 1 ("bpf: Don't explicitly emit BTF for struct btf_iter_num")
  * Add Andrii ack
Patch 2 ("bpf: Introduce task_vma open-coded iterator kfuncs")
  * Mark bpf_iter_task_vma_new args KF_RCU and remove now-unnecessary !task
    check (Yonghong)
    * Although KF_RCU is a function-level flag, in reality it only applies to
      the task_struct *task parameter, as the other two params are a scalar int
      and a specially-handled KF_ARG_PTR_TO_ITER
   * Remove struct bpf_iter_task_vma definition from uapi headers, define in
     kernel/bpf/task_iter.c instead (Andrii)
Patch 3 ("selftests/bpf: Add tests for open-coded task_vma iter")
  * Use a local var when looping over vmas to track map idx. Update vmas_seen
    global after done iterating. Don't start iterating or update vmas_seen if
    vmas_seen global is nonzero. (Andrii)
  * Move getpgid() call to correct spot - above skel detach. (Andrii)

v2 -> v3: https://lore.kernel.org/bpf/[email protected]/

Patch 1 ("bpf: Don't explicitly emit BTF for struct btf_iter_num")
  * Add Yonghong ack

Patch 2 ("bpf: Introduce task_vma open-coded iterator kfuncs")
  * UAPI bpf header and tools/ version should match
  * Add bpf_iter_task_vma_kern_data which bpf_iter_task_vma_kern points to,
    bpf_mem_alloc/free it instead of just vma_iterator. (Alexei)
    * Inner data ptr == NULL implies initialization failed

v1 -> v2: https://lore.kernel.org/bpf/[email protected]/
  * Patch 1
    * Now removes the unnecessary BTF_TYPE_EMIT instead of changing the
      type (Yonghong)
  * Patch 2
    * Don't do unnecessary BTF_TYPE_EMIT (Yonghong)
    * Bump task refcount to prevent ->mm reuse (Yonghong)
    * Keep a pointer to vma_iterator in bpf_iter_task_vma, alloc/free
      via BPF mem allocator (Yonghong, Stanislav)
  * Patch 3

  [0]: https://lore.kernel.org/bpf/[email protected]/
====================

Signed-off-by: Andrii Nakryiko <[email protected]>
  • Loading branch information
anakryiko committed Oct 13, 2023
2 parents ba8ea72 + e0e1a7a commit 0e10fd4
Show file tree
Hide file tree
Showing 8 changed files with 216 additions and 15 deletions.
2 changes: 0 additions & 2 deletions kernel/bpf/bpf_iter.c
Original file line number Diff line number Diff line change
Expand Up @@ -793,8 +793,6 @@ __bpf_kfunc int bpf_iter_num_new(struct bpf_iter_num *it, int start, int end)
BUILD_BUG_ON(sizeof(struct bpf_iter_num_kern) != sizeof(struct bpf_iter_num));
BUILD_BUG_ON(__alignof__(struct bpf_iter_num_kern) != __alignof__(struct bpf_iter_num));

BTF_TYPE_EMIT(struct btf_iter_num);

/* start == end is legit, it's an empty range and we'll just get NULL
* on first (and any subsequent) bpf_iter_num_next() call
*/
Expand Down
3 changes: 3 additions & 0 deletions kernel/bpf/helpers.c
Original file line number Diff line number Diff line change
Expand Up @@ -2552,6 +2552,9 @@ BTF_ID_FLAGS(func, bpf_dynptr_slice_rdwr, KF_RET_NULL)
BTF_ID_FLAGS(func, bpf_iter_num_new, KF_ITER_NEW)
BTF_ID_FLAGS(func, bpf_iter_num_next, KF_ITER_NEXT | KF_RET_NULL)
BTF_ID_FLAGS(func, bpf_iter_num_destroy, KF_ITER_DESTROY)
BTF_ID_FLAGS(func, bpf_iter_task_vma_new, KF_ITER_NEW | KF_RCU)
BTF_ID_FLAGS(func, bpf_iter_task_vma_next, KF_ITER_NEXT | KF_RET_NULL)
BTF_ID_FLAGS(func, bpf_iter_task_vma_destroy, KF_ITER_DESTROY)
BTF_ID_FLAGS(func, bpf_dynptr_adjust)
BTF_ID_FLAGS(func, bpf_dynptr_is_null)
BTF_ID_FLAGS(func, bpf_dynptr_is_rdonly)
Expand Down
91 changes: 91 additions & 0 deletions kernel/bpf/task_iter.c
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@
#include <linux/fs.h>
#include <linux/fdtable.h>
#include <linux/filter.h>
#include <linux/bpf_mem_alloc.h>
#include <linux/btf_ids.h>
#include <linux/mm_types.h>
#include "mmap_unlock_work.h"

static const char * const iter_task_type_names[] = {
Expand Down Expand Up @@ -803,6 +805,95 @@ const struct bpf_func_proto bpf_find_vma_proto = {
.arg5_type = ARG_ANYTHING,
};

struct bpf_iter_task_vma_kern_data {
struct task_struct *task;
struct mm_struct *mm;
struct mmap_unlock_irq_work *work;
struct vma_iterator vmi;
};

struct bpf_iter_task_vma {
/* opaque iterator state; having __u64 here allows to preserve correct
* alignment requirements in vmlinux.h, generated from BTF
*/
__u64 __opaque[1];
} __attribute__((aligned(8)));

/* Non-opaque version of bpf_iter_task_vma */
struct bpf_iter_task_vma_kern {
struct bpf_iter_task_vma_kern_data *data;
} __attribute__((aligned(8)));

__diag_push();
__diag_ignore_all("-Wmissing-prototypes",
"Global functions as their definitions will be in vmlinux BTF");

__bpf_kfunc int bpf_iter_task_vma_new(struct bpf_iter_task_vma *it,
struct task_struct *task, u64 addr)
{
struct bpf_iter_task_vma_kern *kit = (void *)it;
bool irq_work_busy = false;
int err;

BUILD_BUG_ON(sizeof(struct bpf_iter_task_vma_kern) != sizeof(struct bpf_iter_task_vma));
BUILD_BUG_ON(__alignof__(struct bpf_iter_task_vma_kern) != __alignof__(struct bpf_iter_task_vma));

/* is_iter_reg_valid_uninit guarantees that kit hasn't been initialized
* before, so non-NULL kit->data doesn't point to previously
* bpf_mem_alloc'd bpf_iter_task_vma_kern_data
*/
kit->data = bpf_mem_alloc(&bpf_global_ma, sizeof(struct bpf_iter_task_vma_kern_data));
if (!kit->data)
return -ENOMEM;

kit->data->task = get_task_struct(task);
kit->data->mm = task->mm;
if (!kit->data->mm) {
err = -ENOENT;
goto err_cleanup_iter;
}

/* kit->data->work == NULL is valid after bpf_mmap_unlock_get_irq_work */
irq_work_busy = bpf_mmap_unlock_get_irq_work(&kit->data->work);
if (irq_work_busy || !mmap_read_trylock(kit->data->mm)) {
err = -EBUSY;
goto err_cleanup_iter;
}

vma_iter_init(&kit->data->vmi, kit->data->mm, addr);
return 0;

err_cleanup_iter:
if (kit->data->task)
put_task_struct(kit->data->task);
bpf_mem_free(&bpf_global_ma, kit->data);
/* NULL kit->data signals failed bpf_iter_task_vma initialization */
kit->data = NULL;
return err;
}

__bpf_kfunc struct vm_area_struct *bpf_iter_task_vma_next(struct bpf_iter_task_vma *it)
{
struct bpf_iter_task_vma_kern *kit = (void *)it;

if (!kit->data) /* bpf_iter_task_vma_new failed */
return NULL;
return vma_next(&kit->data->vmi);
}

__bpf_kfunc void bpf_iter_task_vma_destroy(struct bpf_iter_task_vma *it)
{
struct bpf_iter_task_vma_kern *kit = (void *)it;

if (kit->data) {
bpf_mmap_unlock_mm(kit->data->work, kit->data->mm);
put_task_struct(kit->data->task);
bpf_mem_free(&bpf_global_ma, kit->data);
}
}

__diag_pop();

DEFINE_PER_CPU(struct mmap_unlock_irq_work, mmap_unlock_work);

static void do_mmap_read_unlock(struct irq_work *entry)
Expand Down
8 changes: 8 additions & 0 deletions tools/testing/selftests/bpf/bpf_experimental.h
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,14 @@ extern void *bpf_percpu_obj_new_impl(__u64 local_type_id, void *meta) __ksym;
*/
extern void bpf_percpu_obj_drop_impl(void *kptr, void *meta) __ksym;

struct bpf_iter_task_vma;

extern int bpf_iter_task_vma_new(struct bpf_iter_task_vma *it,
struct task_struct *task,
unsigned long addr) __ksym;
extern struct vm_area_struct *bpf_iter_task_vma_next(struct bpf_iter_task_vma *it) __ksym;
extern void bpf_iter_task_vma_destroy(struct bpf_iter_task_vma *it) __ksym;

/* Convenience macro to wrap over bpf_obj_drop_impl */
#define bpf_percpu_obj_drop(kptr) bpf_percpu_obj_drop_impl(kptr, NULL)

Expand Down
26 changes: 13 additions & 13 deletions tools/testing/selftests/bpf/prog_tests/bpf_iter.c
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
#include "bpf_iter_task.skel.h"
#include "bpf_iter_task_stack.skel.h"
#include "bpf_iter_task_file.skel.h"
#include "bpf_iter_task_vma.skel.h"
#include "bpf_iter_task_vmas.skel.h"
#include "bpf_iter_task_btf.skel.h"
#include "bpf_iter_tcp4.skel.h"
#include "bpf_iter_tcp6.skel.h"
Expand Down Expand Up @@ -1399,19 +1399,19 @@ static void str_strip_first_line(char *str)
static void test_task_vma_common(struct bpf_iter_attach_opts *opts)
{
int err, iter_fd = -1, proc_maps_fd = -1;
struct bpf_iter_task_vma *skel;
struct bpf_iter_task_vmas *skel;
int len, read_size = 4;
char maps_path[64];

skel = bpf_iter_task_vma__open();
if (!ASSERT_OK_PTR(skel, "bpf_iter_task_vma__open"))
skel = bpf_iter_task_vmas__open();
if (!ASSERT_OK_PTR(skel, "bpf_iter_task_vmas__open"))
return;

skel->bss->pid = getpid();
skel->bss->one_task = opts ? 1 : 0;

err = bpf_iter_task_vma__load(skel);
if (!ASSERT_OK(err, "bpf_iter_task_vma__load"))
err = bpf_iter_task_vmas__load(skel);
if (!ASSERT_OK(err, "bpf_iter_task_vmas__load"))
goto out;

skel->links.proc_maps = bpf_program__attach_iter(
Expand Down Expand Up @@ -1462,25 +1462,25 @@ static void test_task_vma_common(struct bpf_iter_attach_opts *opts)
out:
close(proc_maps_fd);
close(iter_fd);
bpf_iter_task_vma__destroy(skel);
bpf_iter_task_vmas__destroy(skel);
}

static void test_task_vma_dead_task(void)
{
struct bpf_iter_task_vma *skel;
struct bpf_iter_task_vmas *skel;
int wstatus, child_pid = -1;
time_t start_tm, cur_tm;
int err, iter_fd = -1;
int wait_sec = 3;

skel = bpf_iter_task_vma__open();
if (!ASSERT_OK_PTR(skel, "bpf_iter_task_vma__open"))
skel = bpf_iter_task_vmas__open();
if (!ASSERT_OK_PTR(skel, "bpf_iter_task_vmas__open"))
return;

skel->bss->pid = getpid();

err = bpf_iter_task_vma__load(skel);
if (!ASSERT_OK(err, "bpf_iter_task_vma__load"))
err = bpf_iter_task_vmas__load(skel);
if (!ASSERT_OK(err, "bpf_iter_task_vmas__load"))
goto out;

skel->links.proc_maps = bpf_program__attach_iter(
Expand Down Expand Up @@ -1533,7 +1533,7 @@ static void test_task_vma_dead_task(void)
out:
waitpid(child_pid, &wstatus, 0);
close(iter_fd);
bpf_iter_task_vma__destroy(skel);
bpf_iter_task_vmas__destroy(skel);
}

void test_bpf_sockmap_map_iter_fd(void)
Expand Down
58 changes: 58 additions & 0 deletions tools/testing/selftests/bpf/prog_tests/iters.c
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include "iters_looping.skel.h"
#include "iters_num.skel.h"
#include "iters_testmod_seq.skel.h"
#include "iters_task_vma.skel.h"

static void subtest_num_iters(void)
{
Expand Down Expand Up @@ -90,6 +91,61 @@ static void subtest_testmod_seq_iters(void)
iters_testmod_seq__destroy(skel);
}

static void subtest_task_vma_iters(void)
{
unsigned long start, end, bpf_iter_start, bpf_iter_end;
struct iters_task_vma *skel;
char rest_of_line[1000];
unsigned int seen;
FILE *f = NULL;
int err;

skel = iters_task_vma__open_and_load();
if (!ASSERT_OK_PTR(skel, "skel_open_and_load"))
return;

skel->bss->target_pid = getpid();

err = iters_task_vma__attach(skel);
if (!ASSERT_OK(err, "skel_attach"))
goto cleanup;

getpgid(skel->bss->target_pid);
iters_task_vma__detach(skel);

if (!ASSERT_GT(skel->bss->vmas_seen, 0, "vmas_seen_gt_zero"))
goto cleanup;

f = fopen("/proc/self/maps", "r");
if (!ASSERT_OK_PTR(f, "proc_maps_fopen"))
goto cleanup;

seen = 0;
while (fscanf(f, "%lx-%lx %[^\n]\n", &start, &end, rest_of_line) == 3) {
/* [vsyscall] vma isn't _really_ part of task->mm vmas.
* /proc/PID/maps returns it when out of vmas - see get_gate_vma
* calls in fs/proc/task_mmu.c
*/
if (strstr(rest_of_line, "[vsyscall]"))
continue;

bpf_iter_start = skel->bss->vm_ranges[seen].vm_start;
bpf_iter_end = skel->bss->vm_ranges[seen].vm_end;

ASSERT_EQ(bpf_iter_start, start, "vma->vm_start match");
ASSERT_EQ(bpf_iter_end, end, "vma->vm_end match");
seen++;
}

if (!ASSERT_EQ(skel->bss->vmas_seen, seen, "vmas_seen_eq"))
goto cleanup;

cleanup:
if (f)
fclose(f);
iters_task_vma__destroy(skel);
}

void test_iters(void)
{
RUN_TESTS(iters_state_safety);
Expand All @@ -103,4 +159,6 @@ void test_iters(void)
subtest_num_iters();
if (test__start_subtest("testmod_seq"))
subtest_testmod_seq_iters();
if (test__start_subtest("task_vma"))
subtest_task_vma_iters();
}
43 changes: 43 additions & 0 deletions tools/testing/selftests/bpf/progs/iters_task_vma.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
// SPDX-License-Identifier: GPL-2.0
/* Copyright (c) 2023 Meta Platforms, Inc. and affiliates. */

#include "vmlinux.h"
#include "bpf_experimental.h"
#include <bpf/bpf_helpers.h>
#include "bpf_misc.h"

pid_t target_pid = 0;
unsigned int vmas_seen = 0;

struct {
__u64 vm_start;
__u64 vm_end;
} vm_ranges[1000];

SEC("raw_tp/sys_enter")
int iter_task_vma_for_each(const void *ctx)
{
struct task_struct *task = bpf_get_current_task_btf();
struct vm_area_struct *vma;
unsigned int seen = 0;

if (task->pid != target_pid)
return 0;

if (vmas_seen)
return 0;

bpf_for_each(task_vma, vma, task, 0) {
if (seen >= 1000)
break;

vm_ranges[seen].vm_start = vma->vm_start;
vm_ranges[seen].vm_end = vma->vm_end;
seen++;
}

vmas_seen = seen;
return 0;
}

char _license[] SEC("license") = "GPL";

0 comments on commit 0e10fd4

Please sign in to comment.