Currently, unlike other tracing program types, BPF_PROG_TYPE_TRACING is
excluded is_tracing_prog_type checks. This means that they can use maps
containing bpf_spin_lock, bpf_timer, etc. without verification failure.
However, allowing fentry/fexit programs to use maps that have such
bpf_timer in the map value can lead to deadlock.
Suppose that an fentry program is attached to bpf_prog_put, and a TC
program executes and does bpf_map_update_elem on an array map that both
progs share. If the fentry programs calls bpf_map_update_elem for the
same key, it will lead to acquiring of the same lock from within the
critical section protecting the timer.
The call chain is:
bpf_prog_test_run_opts() // TC
bpf_prog_TC
bpf_map_update_elem(array_map, key=0)
bpf_obj_free_fields
bpf_timer_cancel_and_free
__bpf_spin_lock_irqsave
drop_prog_refcnt
bpf_prog_put
bpf_prog_FENTRY // FENTRY
bpf_map_update_elem(array_map, key=0)
bpf_obj_free_fields
bpf_timer_cancel_and_free
__bpf_spin_lock_irqsave // DEADLOCK
BPF_TRACE_ITER attach type can be excluded because it always executes in
process context.
Update selftests using bpf_timer in fentry to TC as they will be broken
by this change.
Fixes: 5e0bc3082e2e ("bpf: Forbid bpf_ktime_get_coarse_ns and bpf_timer_* in tracing progs")
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
---
kernel/bpf/verifier.c | 10 ++++++---
.../testing/selftests/bpf/prog_tests/timer.c | 21 ++++++++++++-------
.../selftests/bpf/prog_tests/timer_crash.c | 9 +++++++-
tools/testing/selftests/bpf/progs/timer.c | 8 +++----
.../testing/selftests/bpf/progs/timer_crash.c | 4 ++--
5 files changed, 34 insertions(+), 18 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index d3b75aa0c54d..6e948c695e84 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -12784,7 +12784,7 @@ static int check_pseudo_btf_id(struct bpf_verifier_env *env,
return err;
}
-static bool is_tracing_prog_type(enum bpf_prog_type type)
+static bool is_tracing_prog_type(enum bpf_prog_type type, enum bpf_attach_type eatype)
{
switch (type) {
case BPF_PROG_TYPE_KPROBE:
@@ -12792,6 +12792,9 @@ static bool is_tracing_prog_type(enum bpf_prog_type type)
case BPF_PROG_TYPE_PERF_EVENT:
case BPF_PROG_TYPE_RAW_TRACEPOINT:
case BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE:
+ case BPF_PROG_TYPE_TRACING:
+ if (eatype == BPF_TRACE_ITER)
+ return false;
return true;
default:
return false;
@@ -12803,6 +12806,7 @@ static int check_map_prog_compatibility(struct bpf_verifier_env *env,
struct bpf_prog *prog)
{
+ enum bpf_attach_type eatype = env->prog->expected_attach_type;
enum bpf_prog_type prog_type = resolve_prog_type(prog);
if (btf_record_has_field(map->record, BPF_SPIN_LOCK)) {
@@ -12811,7 +12815,7 @@ static int check_map_prog_compatibility(struct bpf_verifier_env *env,
return -EINVAL;
}
- if (is_tracing_prog_type(prog_type)) {
+ if (is_tracing_prog_type(prog_type, eatype)) {
verbose(env, "tracing progs cannot use bpf_spin_lock yet\n");
return -EINVAL;
}
@@ -12823,7 +12827,7 @@ static int check_map_prog_compatibility(struct bpf_verifier_env *env,
}
if (btf_record_has_field(map->record, BPF_TIMER)) {
- if (is_tracing_prog_type(prog_type)) {
+ if (is_tracing_prog_type(prog_type, eatype)) {
verbose(env, "tracing progs cannot use bpf_timer yet\n");
return -EINVAL;
}
diff --git a/tools/testing/selftests/bpf/prog_tests/timer.c b/tools/testing/selftests/bpf/prog_tests/timer.c
index 7eb049214859..c0c39da12250 100644
--- a/tools/testing/selftests/bpf/prog_tests/timer.c
+++ b/tools/testing/selftests/bpf/prog_tests/timer.c
@@ -1,25 +1,30 @@
// SPDX-License-Identifier: GPL-2.0
/* Copyright (c) 2021 Facebook */
#include <test_progs.h>
+#include <network_helpers.h>
#include "timer.skel.h"
static int timer(struct timer *timer_skel)
{
+ LIBBPF_OPTS(bpf_test_run_opts, topts,
+ .data_in = &pkt_v4,
+ .data_size_in = sizeof(pkt_v4),
+ .repeat = 1,
+ );
int err, prog_fd;
- LIBBPF_OPTS(bpf_test_run_opts, topts);
-
- err = timer__attach(timer_skel);
- if (!ASSERT_OK(err, "timer_attach"))
- return err;
ASSERT_EQ(timer_skel->data->callback_check, 52, "callback_check1");
ASSERT_EQ(timer_skel->data->callback2_check, 52, "callback2_check1");
prog_fd = bpf_program__fd(timer_skel->progs.test1);
err = bpf_prog_test_run_opts(prog_fd, &topts);
- ASSERT_OK(err, "test_run");
- ASSERT_EQ(topts.retval, 0, "test_run");
- timer__detach(timer_skel);
+ ASSERT_OK(err, "test_run test1");
+ ASSERT_EQ(topts.retval, 0, "test_run retval test1");
+
+ prog_fd = bpf_program__fd(timer_skel->progs.test2);
+ err = bpf_prog_test_run_opts(prog_fd, &topts);
+ ASSERT_OK(err, "test_run test2");
+ ASSERT_EQ(topts.retval, 0, "test_run retval test2");
usleep(50); /* 10 usecs should be enough, but give it extra */
/* check that timer_cb1() was executed 10+10 times */
diff --git a/tools/testing/selftests/bpf/prog_tests/timer_crash.c b/tools/testing/selftests/bpf/prog_tests/timer_crash.c
index f74b82305da8..91f2333b92aa 100644
--- a/tools/testing/selftests/bpf/prog_tests/timer_crash.c
+++ b/tools/testing/selftests/bpf/prog_tests/timer_crash.c
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0
#include <test_progs.h>
+#include <network_helpers.h>
#include "timer_crash.skel.h"
enum {
@@ -9,6 +10,11 @@ enum {
static void test_timer_crash_mode(int mode)
{
+ LIBBPF_OPTS(bpf_test_run_opts, topts,
+ .data_in = &pkt_v4,
+ .data_size_in = sizeof(pkt_v4),
+ .repeat = 1,
+ );
struct timer_crash *skel;
skel = timer_crash__open_and_load();
@@ -18,7 +24,8 @@ static void test_timer_crash_mode(int mode)
skel->bss->crash_map = mode;
if (!ASSERT_OK(timer_crash__attach(skel), "timer_crash__attach"))
goto end;
- usleep(1);
+ ASSERT_OK(bpf_prog_test_run_opts(bpf_program__fd(skel->progs.timer), &topts), "test_run");
+ ASSERT_EQ(topts.retval, 0, "test_run retval");
end:
timer_crash__destroy(skel);
}
diff --git a/tools/testing/selftests/bpf/progs/timer.c b/tools/testing/selftests/bpf/progs/timer.c
index acda5c9cea93..492f62917898 100644
--- a/tools/testing/selftests/bpf/progs/timer.c
+++ b/tools/testing/selftests/bpf/progs/timer.c
@@ -119,8 +119,8 @@ static int timer_cb1(void *map, int *key, struct bpf_timer *timer)
return 0;
}
-SEC("fentry/bpf_fentry_test1")
-int BPF_PROG2(test1, int, a)
+SEC("tc")
+int test1(void *ctx)
{
struct bpf_timer *arr_timer, *lru_timer;
struct elem init = {};
@@ -235,8 +235,8 @@ int bpf_timer_test(void)
return 0;
}
-SEC("fentry/bpf_fentry_test2")
-int BPF_PROG2(test2, int, a, int, b)
+SEC("tc")
+int test2(void *ctx)
{
struct hmap_elem init = {}, *val;
int key = HTAB, key_malloc = HTAB_MALLOC;
diff --git a/tools/testing/selftests/bpf/progs/timer_crash.c b/tools/testing/selftests/bpf/progs/timer_crash.c
index f8f7944e70da..971eb93f485c 100644
--- a/tools/testing/selftests/bpf/progs/timer_crash.c
+++ b/tools/testing/selftests/bpf/progs/timer_crash.c
@@ -26,8 +26,8 @@ struct {
int pid = 0;
int crash_map = 0; /* 0 for amap, 1 for hmap */
-SEC("fentry/do_nanosleep")
-int sys_enter(void *ctx)
+SEC("tc")
+int timer(void *ctx)
{
struct map_elem *e, value = {};
void *map = crash_map ? (void *)&hmap : (void *)&amap;
--
2.38.1
