Currently, a set of core BPF kfuncs (e.g. bpf_task_*, bpf_cgroup_*,
bpf_cpumask_*, etc) cannot be invoked from BPF_PROG_TYPE_SYSCALL
programs. The whitelist approach taken for enabling kfuncs makes sense:
it not safe to call these kfuncs from every program type. For example,
it may not be safe to call bpf_task_acquire() in an fentry to
free_task().
BPF_PROG_TYPE_SYSCALL, on the other hand, is a perfectly safe program
type from which to invoke these kfuncs, as it's a very controlled
environment, and we should never be able to run into any of the typical
problems such as recursive invoations, acquiring references on freeing
kptrs, etc. Being able to invoke these kfuncs would be useful, as
BPF_PROG_TYPE_SYSCALL can be invoked with BPF_PROG_RUN, and would
therefore enable user space programs to synchronously call into BPF to
manipulate these kptrs.
This patch therefore enables invoking the aforementioned core kfuncs
from BPF_PROG_TYPE_SYSCALL progs.
Signed-off-by: David Vernet <[email protected]>
---
kernel/bpf/cpumask.c | 1 +
kernel/bpf/helpers.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/kernel/bpf/cpumask.c b/kernel/bpf/cpumask.c
index dad0fb1c8e87..33c473d676a5 100644
--- a/kernel/bpf/cpumask.c
+++ b/kernel/bpf/cpumask.c
@@ -474,6 +474,7 @@ static int __init cpumask_kfunc_init(void)
ret = bpf_mem_alloc_init(&bpf_cpumask_ma, sizeof(struct bpf_cpumask), false);
ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, &cpumask_kfunc_set);
ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_STRUCT_OPS, &cpumask_kfunc_set);
+ ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SYSCALL, &cpumask_kfunc_set);
return ret ?: register_btf_id_dtor_kfuncs(cpumask_dtors,
ARRAY_SIZE(cpumask_dtors),
THIS_MODULE);
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index d9e7aca8ae9e..8cde717137bd 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -2653,6 +2653,7 @@ static int __init kfunc_init(void)
ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SCHED_CLS, &generic_kfunc_set);
ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_XDP, &generic_kfunc_set);
ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_STRUCT_OPS, &generic_kfunc_set);
+ ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SYSCALL, &generic_kfunc_set);
ret = ret ?: register_btf_id_dtor_kfuncs(generic_dtors,
ARRAY_SIZE(generic_dtors),
THIS_MODULE);
--
2.44.0
Now that we can call some kfuncs from BPF_PROG_TYPE_SYSCALL progs, let's
add some selftests that verify as much. As a bonus, let's also verify
that we can't call the progs from raw tracepoints.
Signed-off-by: David Vernet <[email protected]>
---
.../selftests/bpf/prog_tests/cgrp_kfunc.c | 1 +
.../selftests/bpf/prog_tests/task_kfunc.c | 1 +
.../selftests/bpf/progs/cgrp_kfunc_common.h | 21 +++++++++++++++++++
.../selftests/bpf/progs/cgrp_kfunc_failure.c | 4 ++++
.../selftests/bpf/progs/cgrp_kfunc_success.c | 4 ++++
.../selftests/bpf/progs/cpumask_common.h | 19 +++++++++++++++++
.../selftests/bpf/progs/cpumask_failure.c | 4 ++++
.../selftests/bpf/progs/cpumask_success.c | 3 +++
.../selftests/bpf/progs/task_kfunc_common.h | 18 ++++++++++++++++
.../selftests/bpf/progs/task_kfunc_failure.c | 4 ++++
.../selftests/bpf/progs/task_kfunc_success.c | 4 ++++
11 files changed, 83 insertions(+)
diff --git a/tools/testing/selftests/bpf/prog_tests/cgrp_kfunc.c b/tools/testing/selftests/bpf/prog_tests/cgrp_kfunc.c
index adda85f97058..73f0ec4f4eb7 100644
--- a/tools/testing/selftests/bpf/prog_tests/cgrp_kfunc.c
+++ b/tools/testing/selftests/bpf/prog_tests/cgrp_kfunc.c
@@ -102,6 +102,7 @@ void test_cgrp_kfunc(void)
run_success_test(success_tests[i]);
}
+ RUN_TESTS(cgrp_kfunc_success);
RUN_TESTS(cgrp_kfunc_failure);
cleanup:
diff --git a/tools/testing/selftests/bpf/prog_tests/task_kfunc.c b/tools/testing/selftests/bpf/prog_tests/task_kfunc.c
index d4579f735398..3db4c8601b70 100644
--- a/tools/testing/selftests/bpf/prog_tests/task_kfunc.c
+++ b/tools/testing/selftests/bpf/prog_tests/task_kfunc.c
@@ -94,5 +94,6 @@ void test_task_kfunc(void)
run_success_test(success_tests[i]);
}
+ RUN_TESTS(task_kfunc_success);
RUN_TESTS(task_kfunc_failure);
}
diff --git a/tools/testing/selftests/bpf/progs/cgrp_kfunc_common.h b/tools/testing/selftests/bpf/progs/cgrp_kfunc_common.h
index 22914a70db54..b9972ce4e4dc 100644
--- a/tools/testing/selftests/bpf/progs/cgrp_kfunc_common.h
+++ b/tools/testing/selftests/bpf/progs/cgrp_kfunc_common.h
@@ -27,6 +27,27 @@ struct cgroup *bpf_cgroup_from_id(u64 cgid) __ksym;
void bpf_rcu_read_lock(void) __ksym;
void bpf_rcu_read_unlock(void) __ksym;
+#define CGRP_KFUNC_LOAD_TEST(__name) \
+int BPF_PROG(cgroup_kfunc_load_test_##__name) \
+{ \
+ struct cgroup *cgrp, *ref; \
+ \
+ cgrp = bpf_cgroup_from_id(0); \
+ if (!cgrp) \
+ return 0; \
+ ref = bpf_cgroup_acquire(cgrp); \
+ if (!ref) { \
+ bpf_cgroup_release(cgrp); \
+ return 0; \
+ } \
+ \
+ bpf_cgroup_release(ref); \
+ bpf_cgroup_release(cgrp); \
+ \
+ return 0; \
+}
+
+
static inline struct __cgrps_kfunc_map_value *cgrps_kfunc_map_value_lookup(struct cgroup *cgrp)
{
s32 id;
diff --git a/tools/testing/selftests/bpf/progs/cgrp_kfunc_failure.c b/tools/testing/selftests/bpf/progs/cgrp_kfunc_failure.c
index 9fe9c4a4e8f6..ff67d4632dfa 100644
--- a/tools/testing/selftests/bpf/progs/cgrp_kfunc_failure.c
+++ b/tools/testing/selftests/bpf/progs/cgrp_kfunc_failure.c
@@ -245,3 +245,7 @@ int BPF_PROG(cgrp_kfunc_release_unacquired, struct cgroup *cgrp, const char *pat
return 0;
}
+
+SEC("raw_tp")
+__failure __msg("calling kernel function")
+CGRP_KFUNC_LOAD_TEST(raw_tp)
diff --git a/tools/testing/selftests/bpf/progs/cgrp_kfunc_success.c b/tools/testing/selftests/bpf/progs/cgrp_kfunc_success.c
index 5354455a01be..ff1beb29f3f5 100644
--- a/tools/testing/selftests/bpf/progs/cgrp_kfunc_success.c
+++ b/tools/testing/selftests/bpf/progs/cgrp_kfunc_success.c
@@ -5,6 +5,7 @@
#include <bpf/bpf_tracing.h>
#include <bpf/bpf_helpers.h>
+#include "bpf_misc.h"
#include "cgrp_kfunc_common.h"
char _license[] SEC("license") = "GPL";
@@ -221,3 +222,6 @@ int BPF_PROG(test_cgrp_from_id, struct cgroup *cgrp, const char *path)
return 0;
}
+
+SEC("syscall") __success
+CGRP_KFUNC_LOAD_TEST(syscall)
diff --git a/tools/testing/selftests/bpf/progs/cpumask_common.h b/tools/testing/selftests/bpf/progs/cpumask_common.h
index c705d8112a35..5178d62c5c9f 100644
--- a/tools/testing/selftests/bpf/progs/cpumask_common.h
+++ b/tools/testing/selftests/bpf/progs/cpumask_common.h
@@ -60,6 +60,25 @@ u32 bpf_cpumask_weight(const struct cpumask *cpumask) __ksym __weak;
void bpf_rcu_read_lock(void) __ksym __weak;
void bpf_rcu_read_unlock(void) __ksym __weak;
+#define CPUMASK_KFUNC_LOAD_TEST(__name) \
+int BPF_PROG(cpumask_kfunc_load_test_##__name) \
+{ \
+ struct bpf_cpumask *alloc, *ref; \
+ \
+ alloc = bpf_cpumask_create(); \
+ if (!alloc) \
+ return 0; \
+ \
+ ref = bpf_cpumask_acquire(alloc); \
+ if (ref) \
+ bpf_cpumask_release(ref); \
+ bpf_cpumask_set_cpu(0, alloc); \
+ bpf_cpumask_test_cpu(0, (const struct cpumask *)alloc); \
+ bpf_cpumask_release(alloc); \
+ \
+ return 0; \
+}
+
static inline const struct cpumask *cast(struct bpf_cpumask *cpumask)
{
return (const struct cpumask *)cpumask;
diff --git a/tools/testing/selftests/bpf/progs/cpumask_failure.c b/tools/testing/selftests/bpf/progs/cpumask_failure.c
index a9bf6ea336cf..55815df8000f 100644
--- a/tools/testing/selftests/bpf/progs/cpumask_failure.c
+++ b/tools/testing/selftests/bpf/progs/cpumask_failure.c
@@ -190,3 +190,7 @@ int BPF_PROG(test_global_mask_rcu_no_null_check, struct task_struct *task, u64 c
return 0;
}
+
+SEC("raw_tp")
+__failure __msg("calling kernel function")
+CPUMASK_KFUNC_LOAD_TEST(raw_tp)
diff --git a/tools/testing/selftests/bpf/progs/cpumask_success.c b/tools/testing/selftests/bpf/progs/cpumask_success.c
index 7a1e64c6c065..a4f32a5a26d1 100644
--- a/tools/testing/selftests/bpf/progs/cpumask_success.c
+++ b/tools/testing/selftests/bpf/progs/cpumask_success.c
@@ -525,3 +525,6 @@ int BPF_PROG(test_refcount_null_tracking, struct task_struct *task, u64 clone_fl
bpf_cpumask_release(mask2);
return 0;
}
+
+SEC("syscall") __success
+CPUMASK_KFUNC_LOAD_TEST(syscall)
diff --git a/tools/testing/selftests/bpf/progs/task_kfunc_common.h b/tools/testing/selftests/bpf/progs/task_kfunc_common.h
index 41f2d44f49cb..2278325ca902 100644
--- a/tools/testing/selftests/bpf/progs/task_kfunc_common.h
+++ b/tools/testing/selftests/bpf/progs/task_kfunc_common.h
@@ -26,6 +26,24 @@ struct task_struct *bpf_task_from_pid(s32 pid) __ksym;
void bpf_rcu_read_lock(void) __ksym;
void bpf_rcu_read_unlock(void) __ksym;
+#define TASK_KFUNC_LOAD_TEST(__name) \
+int BPF_PROG(task_kfunc_load_test_##__name) \
+{ \
+ struct task_struct *current, *ref_1, *ref_2; \
+ \
+ current = bpf_get_current_task_btf(); \
+ ref_1 = bpf_task_from_pid(current->pid); \
+ if (!ref_1) \
+ return 0; \
+ \
+ ref_2 = bpf_task_acquire(ref_1); \
+ if (ref_2) \
+ bpf_task_release(ref_2); \
+ bpf_task_release(ref_1); \
+ \
+ return 0; \
+}
+
static inline struct __tasks_kfunc_map_value *tasks_kfunc_map_value_lookup(struct task_struct *p)
{
s32 pid;
diff --git a/tools/testing/selftests/bpf/progs/task_kfunc_failure.c b/tools/testing/selftests/bpf/progs/task_kfunc_failure.c
index ad88a3796ddf..57ec25463d80 100644
--- a/tools/testing/selftests/bpf/progs/task_kfunc_failure.c
+++ b/tools/testing/selftests/bpf/progs/task_kfunc_failure.c
@@ -324,3 +324,7 @@ int BPF_PROG(task_kfunc_release_in_map, struct task_struct *task, u64 clone_flag
return 0;
}
+
+SEC("raw_tp")
+__failure __msg("calling kernel function")
+TASK_KFUNC_LOAD_TEST(raw_tp)
diff --git a/tools/testing/selftests/bpf/progs/task_kfunc_success.c b/tools/testing/selftests/bpf/progs/task_kfunc_success.c
index 70df695312dc..203ff461a92c 100644
--- a/tools/testing/selftests/bpf/progs/task_kfunc_success.c
+++ b/tools/testing/selftests/bpf/progs/task_kfunc_success.c
@@ -5,6 +5,7 @@
#include <bpf/bpf_tracing.h>
#include <bpf/bpf_helpers.h>
+#include "bpf_misc.h"
#include "task_kfunc_common.h"
char _license[] SEC("license") = "GPL";
@@ -314,3 +315,6 @@ int BPF_PROG(task_kfunc_acquire_trusted_walked, struct task_struct *task, u64 cl
return 0;
}
+
+SEC("syscall") __success
+TASK_KFUNC_LOAD_TEST(syscall)
--
2.44.0
On 4/3/24 6:03 PM, David Vernet wrote:
> Currently, a set of core BPF kfuncs (e.g. bpf_task_*, bpf_cgroup_*,
> bpf_cpumask_*, etc) cannot be invoked from BPF_PROG_TYPE_SYSCALL
> programs. The whitelist approach taken for enabling kfuncs makes sense:
> it not safe to call these kfuncs from every program type. For example,
> it may not be safe to call bpf_task_acquire() in an fentry to
> free_task().
>
> BPF_PROG_TYPE_SYSCALL, on the other hand, is a perfectly safe program
> type from which to invoke these kfuncs, as it's a very controlled
> environment, and we should never be able to run into any of the typical
> problems such as recursive invoations, acquiring references on freeing
> kptrs, etc. Being able to invoke these kfuncs would be useful, as
> BPF_PROG_TYPE_SYSCALL can be invoked with BPF_PROG_RUN, and would
> therefore enable user space programs to synchronously call into BPF to
> manipulate these kptrs.
>
> This patch therefore enables invoking the aforementioned core kfuncs
> from BPF_PROG_TYPE_SYSCALL progs.
>
> Signed-off-by: David Vernet <[email protected]>
Acked-by: Yonghong Song <[email protected]>
On Thu, Apr 04, 2024 at 09:04:19AM -0700, Yonghong Song wrote:
>
> On 4/3/24 6:03 PM, David Vernet wrote:
> > Now that we can call some kfuncs from BPF_PROG_TYPE_SYSCALL progs, let's
> > add some selftests that verify as much. As a bonus, let's also verify
> > that we can't call the progs from raw tracepoints.
> >
> > Signed-off-by: David Vernet <[email protected]>
>
> Ack with some comments below.
>
> Acked-by: Yonghong Song <[email protected]>
Thanks for the review. It looks like accidentally replied directly to
me, so I'll re-add the missing cc's.
>
> > ---
> > .../selftests/bpf/prog_tests/cgrp_kfunc.c | 1 +
> > .../selftests/bpf/prog_tests/task_kfunc.c | 1 +
> > .../selftests/bpf/progs/cgrp_kfunc_common.h | 21 +++++++++++++++++++
> > .../selftests/bpf/progs/cgrp_kfunc_failure.c | 4 ++++
> > .../selftests/bpf/progs/cgrp_kfunc_success.c | 4 ++++
> > .../selftests/bpf/progs/cpumask_common.h | 19 +++++++++++++++++
> > .../selftests/bpf/progs/cpumask_failure.c | 4 ++++
> > .../selftests/bpf/progs/cpumask_success.c | 3 +++
> > .../selftests/bpf/progs/task_kfunc_common.h | 18 ++++++++++++++++
> > .../selftests/bpf/progs/task_kfunc_failure.c | 4 ++++
> > .../selftests/bpf/progs/task_kfunc_success.c | 4 ++++
> > 11 files changed, 83 insertions(+)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/cgrp_kfunc.c b/tools/testing/selftests/bpf/prog_tests/cgrp_kfunc.c
> > index adda85f97058..73f0ec4f4eb7 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/cgrp_kfunc.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/cgrp_kfunc.c
> > @@ -102,6 +102,7 @@ void test_cgrp_kfunc(void)
> > run_success_test(success_tests[i]);
> > }
> > + RUN_TESTS(cgrp_kfunc_success);
> > RUN_TESTS(cgrp_kfunc_failure);
> > cleanup:
> > diff --git a/tools/testing/selftests/bpf/prog_tests/task_kfunc.c b/tools/testing/selftests/bpf/prog_tests/task_kfunc.c
> > index d4579f735398..3db4c8601b70 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/task_kfunc.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/task_kfunc.c
> > @@ -94,5 +94,6 @@ void test_task_kfunc(void)
> > run_success_test(success_tests[i]);
> > }
> > + RUN_TESTS(task_kfunc_success);
> > RUN_TESTS(task_kfunc_failure);
> > }
>
> The above RUN_TESTS(cgrp_kfunc_success) and RUN_TESTS(task_kfunc_success)
> will do duplicate work for *existing* bpf programs in their respective
> files. I think we still prefer to have cgrp_kfunc_success tests
> in cgrp_kfunc.c to make it easy to cross check. But in order to
> remove duplicate work, one option is to make other non-RUN_TESTS
> programs in those files not auto-loaded and their corresponding
> prog_tests/*.c file need to explicitly enable loading the problem.
Good point, and yes I agree with that approach of not auto-loading
non-RUN_TESTS programs. Considering that we have a __success BTF tag to
say, "this prog should successfully load", it seems odd that we'd also
automatically load and validate progs that _didn't_ specify that tag as
well. At that point, I'm not sure what value the tag is bringing. Also,
that was the expected behavior before RUN_TESTS() was introduced, so it
hopefully shouldn't cause much if any churn.
> Maybe the current patch is okay even with duplicated work as it
> should not take much time to verify those tiny problems.
IMO it should be fine for now as the overhead for validating and loading
these progs is low, but it'd definitely be good to address this problem
in a follow-up. I don't think it should take too much effort -- AFAICT
we'd just have to mark a test spec as invalid if it didn't have any BTF
test tags. Ideally I'd like to separate that from this patch set, but I
can do it here if folks want.
Thanks,
David
On 4/4/24 9:33 AM, David Vernet wrote:
> On Thu, Apr 04, 2024 at 09:04:19AM -0700, Yonghong Song wrote:
>> On 4/3/24 6:03 PM, David Vernet wrote:
>>> Now that we can call some kfuncs from BPF_PROG_TYPE_SYSCALL progs, let's
>>> add some selftests that verify as much. As a bonus, let's also verify
>>> that we can't call the progs from raw tracepoints.
>>>
>>> Signed-off-by: David Vernet <[email protected]>
>> Ack with some comments below.
>>
>> Acked-by: Yonghong Song <[email protected]>
> Thanks for the review. It looks like accidentally replied directly to
> me, so I'll re-add the missing cc's.
>
>>> ---
>>> .../selftests/bpf/prog_tests/cgrp_kfunc.c | 1 +
>>> .../selftests/bpf/prog_tests/task_kfunc.c | 1 +
>>> .../selftests/bpf/progs/cgrp_kfunc_common.h | 21 +++++++++++++++++++
>>> .../selftests/bpf/progs/cgrp_kfunc_failure.c | 4 ++++
>>> .../selftests/bpf/progs/cgrp_kfunc_success.c | 4 ++++
>>> .../selftests/bpf/progs/cpumask_common.h | 19 +++++++++++++++++
>>> .../selftests/bpf/progs/cpumask_failure.c | 4 ++++
>>> .../selftests/bpf/progs/cpumask_success.c | 3 +++
>>> .../selftests/bpf/progs/task_kfunc_common.h | 18 ++++++++++++++++
>>> .../selftests/bpf/progs/task_kfunc_failure.c | 4 ++++
>>> .../selftests/bpf/progs/task_kfunc_success.c | 4 ++++
>>> 11 files changed, 83 insertions(+)
>>>
>>> diff --git a/tools/testing/selftests/bpf/prog_tests/cgrp_kfunc.c b/tools/testing/selftests/bpf/prog_tests/cgrp_kfunc.c
>>> index adda85f97058..73f0ec4f4eb7 100644
>>> --- a/tools/testing/selftests/bpf/prog_tests/cgrp_kfunc.c
>>> +++ b/tools/testing/selftests/bpf/prog_tests/cgrp_kfunc.c
>>> @@ -102,6 +102,7 @@ void test_cgrp_kfunc(void)
>>> run_success_test(success_tests[i]);
>>> }
>>> + RUN_TESTS(cgrp_kfunc_success);
>>> RUN_TESTS(cgrp_kfunc_failure);
>>> cleanup:
>>> diff --git a/tools/testing/selftests/bpf/prog_tests/task_kfunc.c b/tools/testing/selftests/bpf/prog_tests/task_kfunc.c
>>> index d4579f735398..3db4c8601b70 100644
>>> --- a/tools/testing/selftests/bpf/prog_tests/task_kfunc.c
>>> +++ b/tools/testing/selftests/bpf/prog_tests/task_kfunc.c
>>> @@ -94,5 +94,6 @@ void test_task_kfunc(void)
>>> run_success_test(success_tests[i]);
>>> }
>>> + RUN_TESTS(task_kfunc_success);
>>> RUN_TESTS(task_kfunc_failure);
>>> }
>> The above RUN_TESTS(cgrp_kfunc_success) and RUN_TESTS(task_kfunc_success)
>> will do duplicate work for *existing* bpf programs in their respective
>> files. I think we still prefer to have cgrp_kfunc_success tests
>> in cgrp_kfunc.c to make it easy to cross check. But in order to
>> remove duplicate work, one option is to make other non-RUN_TESTS
>> programs in those files not auto-loaded and their corresponding
>> prog_tests/*.c file need to explicitly enable loading the problem.
> Good point, and yes I agree with that approach of not auto-loading
> non-RUN_TESTS programs. Considering that we have a __success BTF tag to
> say, "this prog should successfully load", it seems odd that we'd also
> automatically load and validate progs that _didn't_ specify that tag as
> well. At that point, I'm not sure what value the tag is bringing. Also,
> that was the expected behavior before RUN_TESTS() was introduced, so it
> hopefully shouldn't cause much if any churn.
>
>> Maybe the current patch is okay even with duplicated work as it
>> should not take much time to verify those tiny problems.
> IMO it should be fine for now as the overhead for validating and loading
> these progs is low, but it'd definitely be good to address this problem
> in a follow-up. I don't think it should take too much effort -- AFAICT
> we'd just have to mark a test spec as invalid if it didn't have any BTF
> test tags. Ideally I'd like to separate that from this patch set, but I
> can do it here if folks want.
Or you can remove __success from cgrp_kfunc_success.c, cpumask_success.c
and task_kfunc_success.c and also remove their corresponding
RUN_TESTS(cgrp_kfunc_success) and RUN_TESTS(task_kfunc_success).
For example, you do not have RUN_TESTS(cpumask_success) and it is okay.
Basically, those expect-to-succeed new programs should be verified
already even without RUN_TESTS.
>
> Thanks,
> David
On Thu, Apr 4, 2024 at 9:33 AM David Vernet <[email protected]> wrote:
>
> On Thu, Apr 04, 2024 at 09:04:19AM -0700, Yonghong Song wrote:
> >
> > On 4/3/24 6:03 PM, David Vernet wrote:
> > > Now that we can call some kfuncs from BPF_PROG_TYPE_SYSCALL progs, let's
> > > add some selftests that verify as much. As a bonus, let's also verify
> > > that we can't call the progs from raw tracepoints.
> > >
> > > Signed-off-by: David Vernet <[email protected]>
> >
> > Ack with some comments below.
> >
> > Acked-by: Yonghong Song <[email protected]>
>
> Thanks for the review. It looks like accidentally replied directly to
> me, so I'll re-add the missing cc's.
>
And dropped bpf@vger :) adding back
> >
> > > ---
> > > .../selftests/bpf/prog_tests/cgrp_kfunc.c | 1 +
> > > .../selftests/bpf/prog_tests/task_kfunc.c | 1 +
> > > .../selftests/bpf/progs/cgrp_kfunc_common.h | 21 +++++++++++++++++++
> > > .../selftests/bpf/progs/cgrp_kfunc_failure.c | 4 ++++
> > > .../selftests/bpf/progs/cgrp_kfunc_success.c | 4 ++++
> > > .../selftests/bpf/progs/cpumask_common.h | 19 +++++++++++++++++
> > > .../selftests/bpf/progs/cpumask_failure.c | 4 ++++
> > > .../selftests/bpf/progs/cpumask_success.c | 3 +++
> > > .../selftests/bpf/progs/task_kfunc_common.h | 18 ++++++++++++++++
> > > .../selftests/bpf/progs/task_kfunc_failure.c | 4 ++++
> > > .../selftests/bpf/progs/task_kfunc_success.c | 4 ++++
> > > 11 files changed, 83 insertions(+)
> > >
> > > diff --git a/tools/testing/selftests/bpf/prog_tests/cgrp_kfunc.c b/tools/testing/selftests/bpf/prog_tests/cgrp_kfunc.c
> > > index adda85f97058..73f0ec4f4eb7 100644
> > > --- a/tools/testing/selftests/bpf/prog_tests/cgrp_kfunc.c
> > > +++ b/tools/testing/selftests/bpf/prog_tests/cgrp_kfunc.c
> > > @@ -102,6 +102,7 @@ void test_cgrp_kfunc(void)
> > > run_success_test(success_tests[i]);
> > > }
> > > + RUN_TESTS(cgrp_kfunc_success);
> > > RUN_TESTS(cgrp_kfunc_failure);
> > > cleanup:
> > > diff --git a/tools/testing/selftests/bpf/prog_tests/task_kfunc.c b/tools/testing/selftests/bpf/prog_tests/task_kfunc.c
> > > index d4579f735398..3db4c8601b70 100644
> > > --- a/tools/testing/selftests/bpf/prog_tests/task_kfunc.c
> > > +++ b/tools/testing/selftests/bpf/prog_tests/task_kfunc.c
> > > @@ -94,5 +94,6 @@ void test_task_kfunc(void)
> > > run_success_test(success_tests[i]);
> > > }
> > > + RUN_TESTS(task_kfunc_success);
> > > RUN_TESTS(task_kfunc_failure);
> > > }
> >
> > The above RUN_TESTS(cgrp_kfunc_success) and RUN_TESTS(task_kfunc_success)
> > will do duplicate work for *existing* bpf programs in their respective
> > files. I think we still prefer to have cgrp_kfunc_success tests
> > in cgrp_kfunc.c to make it easy to cross check. But in order to
> > remove duplicate work, one option is to make other non-RUN_TESTS
> > programs in those files not auto-loaded and their corresponding
> > prog_tests/*.c file need to explicitly enable loading the problem.
>
> Good point, and yes I agree with that approach of not auto-loading
> non-RUN_TESTS programs. Considering that we have a __success BTF tag to
> say, "this prog should successfully load", it seems odd that we'd also
> automatically load and validate progs that _didn't_ specify that tag as
> well. At that point, I'm not sure what value the tag is bringing. Also,
Just more explicitness (if desired). Normally __success would be
augmented by __msg() or __retval(). I'd feel uncomfortable just
silently skipping programs that are not marked with __success, as it
would be too easy to accidentally forget to add it and not know that
the BPF program is not tested.
I'd say that RUN_TESTS-based programs should be kept separate from any
other BPF programs that have a custom user-space testing part, though.
About the patch itself. I don't really see much point in adding
*_KFUNC_LOAD_TEST macros. They are used once or twice in total, while
obscuring *what* is actually being tested. Unless you expect to add 5+
more copies of them, I'd just inline them explicitly.
> that was the expected behavior before RUN_TESTS() was introduced, so it
> hopefully shouldn't cause much if any churn.
>
> > Maybe the current patch is okay even with duplicated work as it
> > should not take much time to verify those tiny problems.
>
> IMO it should be fine for now as the overhead for validating and loading
> these progs is low, but it'd definitely be good to address this problem
> in a follow-up. I don't think it should take too much effort -- AFAICT
> we'd just have to mark a test spec as invalid if it didn't have any BTF
> test tags. Ideally I'd like to separate that from this patch set, but I
> can do it here if folks want.
>
> Thanks,
> David
On Thu, Apr 04, 2024 at 03:16:56PM -0700, Andrii Nakryiko wrote:
> On Thu, Apr 4, 2024 at 9:33 AM David Vernet <[email protected]> wrote:
> >
> > On Thu, Apr 04, 2024 at 09:04:19AM -0700, Yonghong Song wrote:
> > >
> > > On 4/3/24 6:03 PM, David Vernet wrote:
> > > > Now that we can call some kfuncs from BPF_PROG_TYPE_SYSCALL progs, let's
> > > > add some selftests that verify as much. As a bonus, let's also verify
> > > > that we can't call the progs from raw tracepoints.
> > > >
> > > > Signed-off-by: David Vernet <[email protected]>
> > >
> > > Ack with some comments below.
> > >
> > > Acked-by: Yonghong Song <[email protected]>
> >
> > Thanks for the review. It looks like accidentally replied directly to
> > me, so I'll re-add the missing cc's.
> >
>
> And dropped bpf@vger :) adding back
*facepalm*, thanks!
>
> > >
> > > > ---
> > > > .../selftests/bpf/prog_tests/cgrp_kfunc.c | 1 +
> > > > .../selftests/bpf/prog_tests/task_kfunc.c | 1 +
> > > > .../selftests/bpf/progs/cgrp_kfunc_common.h | 21 +++++++++++++++++++
> > > > .../selftests/bpf/progs/cgrp_kfunc_failure.c | 4 ++++
> > > > .../selftests/bpf/progs/cgrp_kfunc_success.c | 4 ++++
> > > > .../selftests/bpf/progs/cpumask_common.h | 19 +++++++++++++++++
> > > > .../selftests/bpf/progs/cpumask_failure.c | 4 ++++
> > > > .../selftests/bpf/progs/cpumask_success.c | 3 +++
> > > > .../selftests/bpf/progs/task_kfunc_common.h | 18 ++++++++++++++++
> > > > .../selftests/bpf/progs/task_kfunc_failure.c | 4 ++++
> > > > .../selftests/bpf/progs/task_kfunc_success.c | 4 ++++
> > > > 11 files changed, 83 insertions(+)
> > > >
> > > > diff --git a/tools/testing/selftests/bpf/prog_tests/cgrp_kfunc.c b/tools/testing/selftests/bpf/prog_tests/cgrp_kfunc.c
> > > > index adda85f97058..73f0ec4f4eb7 100644
> > > > --- a/tools/testing/selftests/bpf/prog_tests/cgrp_kfunc.c
> > > > +++ b/tools/testing/selftests/bpf/prog_tests/cgrp_kfunc.c
> > > > @@ -102,6 +102,7 @@ void test_cgrp_kfunc(void)
> > > > run_success_test(success_tests[i]);
> > > > }
> > > > + RUN_TESTS(cgrp_kfunc_success);
> > > > RUN_TESTS(cgrp_kfunc_failure);
> > > > cleanup:
> > > > diff --git a/tools/testing/selftests/bpf/prog_tests/task_kfunc.c b/tools/testing/selftests/bpf/prog_tests/task_kfunc.c
> > > > index d4579f735398..3db4c8601b70 100644
> > > > --- a/tools/testing/selftests/bpf/prog_tests/task_kfunc.c
> > > > +++ b/tools/testing/selftests/bpf/prog_tests/task_kfunc.c
> > > > @@ -94,5 +94,6 @@ void test_task_kfunc(void)
> > > > run_success_test(success_tests[i]);
> > > > }
> > > > + RUN_TESTS(task_kfunc_success);
> > > > RUN_TESTS(task_kfunc_failure);
> > > > }
> > >
> > > The above RUN_TESTS(cgrp_kfunc_success) and RUN_TESTS(task_kfunc_success)
> > > will do duplicate work for *existing* bpf programs in their respective
> > > files. I think we still prefer to have cgrp_kfunc_success tests
> > > in cgrp_kfunc.c to make it easy to cross check. But in order to
> > > remove duplicate work, one option is to make other non-RUN_TESTS
> > > programs in those files not auto-loaded and their corresponding
> > > prog_tests/*.c file need to explicitly enable loading the problem.
> >
> > Good point, and yes I agree with that approach of not auto-loading
> > non-RUN_TESTS programs. Considering that we have a __success BTF tag to
> > say, "this prog should successfully load", it seems odd that we'd also
> > automatically load and validate progs that _didn't_ specify that tag as
> > well. At that point, I'm not sure what value the tag is bringing. Also,
>
> Just more explicitness (if desired). Normally __success would be
> augmented by __msg() or __retval(). I'd feel uncomfortable just
But __success really has no actual purpose, right? Isn't it identical to
if it's just left off? You don't need __success to specify __msg() or
__retval() right?
> silently skipping programs that are not marked with __success, as it
> would be too easy to accidentally forget to add it and not know that
> the BPF program is not tested.
>
> I'd say that RUN_TESTS-based programs should be kept separate from any
> other BPF programs that have a custom user-space testing part, though.
IF we do go this way, maybe just a __skip or something tag would be
sufficient?
> About the patch itself. I don't really see much point in adding
> *_KFUNC_LOAD_TEST macros. They are used once or twice in total, while
> obscuring *what* is actually being tested. Unless you expect to add 5+
> more copies of them, I'd just inline them explicitly.
It's not really important what's in the actual prog though -- the point
is that we're verifying we can invoke some kfuncs in a certain prog
type. But yes, it does obscure what's there, and I'm fine with
copy-pasting them if that's your preference. The reason I went with a
macro was to make it easy for us to quickly test new prog types as we
add support for them, or to add other negative testcases for unsafe prog
types. Right now we're just testing tracing progs.
>
> > that was the expected behavior before RUN_TESTS() was introduced, so it
> > hopefully shouldn't cause much if any churn.
> >
> > > Maybe the current patch is okay even with duplicated work as it
> > > should not take much time to verify those tiny problems.
> >
> > IMO it should be fine for now as the overhead for validating and loading
> > these progs is low, but it'd definitely be good to address this problem
> > in a follow-up. I don't think it should take too much effort -- AFAICT
> > we'd just have to mark a test spec as invalid if it didn't have any BTF
> > test tags. Ideally I'd like to separate that from this patch set, but I
> > can do it here if folks want.
> >
> > Thanks,
> > David
On Wed, Apr 3, 2024 at 6:03 PM David Vernet <[email protected]> wrote:
>
> Currently, a set of core BPF kfuncs (e.g. bpf_task_*, bpf_cgroup_*,
> bpf_cpumask_*, etc) cannot be invoked from BPF_PROG_TYPE_SYSCALL
> programs. The whitelist approach taken for enabling kfuncs makes sense:
> it not safe to call these kfuncs from every program type. For example,
> it may not be safe to call bpf_task_acquire() in an fentry to
> free_task().
>
> BPF_PROG_TYPE_SYSCALL, on the other hand, is a perfectly safe program
> type from which to invoke these kfuncs, as it's a very controlled
> environment, and we should never be able to run into any of the typical
> problems such as recursive invoations, acquiring references on freeing
> kptrs, etc. Being able to invoke these kfuncs would be useful, as
> BPF_PROG_TYPE_SYSCALL can be invoked with BPF_PROG_RUN, and would
> therefore enable user space programs to synchronously call into BPF to
> manipulate these kptrs.
>
> This patch therefore enables invoking the aforementioned core kfuncs
> from BPF_PROG_TYPE_SYSCALL progs.
>
> Signed-off-by: David Vernet <[email protected]>
> ---
> kernel/bpf/cpumask.c | 1 +
> kernel/bpf/helpers.c | 1 +
> 2 files changed, 2 insertions(+)
>
Makes sense, but see my comments on patch #2.
Acked-by: Andrii Nakryiko <[email protected]>
> diff --git a/kernel/bpf/cpumask.c b/kernel/bpf/cpumask.c
> index dad0fb1c8e87..33c473d676a5 100644
> --- a/kernel/bpf/cpumask.c
> +++ b/kernel/bpf/cpumask.c
> @@ -474,6 +474,7 @@ static int __init cpumask_kfunc_init(void)
> ret = bpf_mem_alloc_init(&bpf_cpumask_ma, sizeof(struct bpf_cpumask), false);
> ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, &cpumask_kfunc_set);
> ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_STRUCT_OPS, &cpumask_kfunc_set);
> + ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SYSCALL, &cpumask_kfunc_set);
> return ret ?: register_btf_id_dtor_kfuncs(cpumask_dtors,
> ARRAY_SIZE(cpumask_dtors),
> THIS_MODULE);
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index d9e7aca8ae9e..8cde717137bd 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -2653,6 +2653,7 @@ static int __init kfunc_init(void)
> ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SCHED_CLS, &generic_kfunc_set);
> ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_XDP, &generic_kfunc_set);
> ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_STRUCT_OPS, &generic_kfunc_set);
> + ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SYSCALL, &generic_kfunc_set);
> ret = ret ?: register_btf_id_dtor_kfuncs(generic_dtors,
> ARRAY_SIZE(generic_dtors),
> THIS_MODULE);
> --
> 2.44.0
>
On Thu, Apr 04, 2024 at 03:35:32PM -0700, Andrii Nakryiko wrote:
[...]
> > > > > The above RUN_TESTS(cgrp_kfunc_success) and RUN_TESTS(task_kfunc_success)
> > > > > will do duplicate work for *existing* bpf programs in their respective
> > > > > files. I think we still prefer to have cgrp_kfunc_success tests
> > > > > in cgrp_kfunc.c to make it easy to cross check. But in order to
> > > > > remove duplicate work, one option is to make other non-RUN_TESTS
> > > > > programs in those files not auto-loaded and their corresponding
> > > > > prog_tests/*.c file need to explicitly enable loading the problem.
> > > >
> > > > Good point, and yes I agree with that approach of not auto-loading
> > > > non-RUN_TESTS programs. Considering that we have a __success BTF tag to
> > > > say, "this prog should successfully load", it seems odd that we'd also
> > > > automatically load and validate progs that _didn't_ specify that tag as
> > > > well. At that point, I'm not sure what value the tag is bringing. Also,
> > >
> > > Just more explicitness (if desired). Normally __success would be
> > > augmented by __msg() or __retval(). I'd feel uncomfortable just
> >
> > But __success really has no actual purpose, right? Isn't it identical to
> > if it's just left off? You don't need __success to specify __msg() or
> > __retval() right?
>
> right, it's just a more explicit documentation-like annotation, if you will
>
> >
> > > silently skipping programs that are not marked with __success, as it
> > > would be too easy to accidentally forget to add it and not know that
> > > the BPF program is not tested.
> > >
> > > I'd say that RUN_TESTS-based programs should be kept separate from any
> > > other BPF programs that have a custom user-space testing part, though.
> >
> > IF we do go this way, maybe just a __skip or something tag would be
> > sufficient?
>
> if we go this way we wouldn't need __skip, but if we do not go, then
> sure, why not. But in general, __skip makes sense either way, I guess,
> I have no problem with it.
Sorry, by "if we go this way" what I meant was "if we continue to have
RUN_TESTS() run all progs by default." Given that we're doing that, it
sounds like we're on the same page page and that __skip is the way to
go.
>
> >
> > > About the patch itself. I don't really see much point in adding
> > > *_KFUNC_LOAD_TEST macros. They are used once or twice in total, while
> > > obscuring *what* is actually being tested. Unless you expect to add 5+
> > > more copies of them, I'd just inline them explicitly.
> >
> > It's not really important what's in the actual prog though -- the point
> > is that we're verifying we can invoke some kfuncs in a certain prog
> > type. But yes, it does obscure what's there, and I'm fine with
> > copy-pasting them if that's your preference. The reason I went with a
> > macro was to make it easy for us to quickly test new prog types as we
> > add support for them, or to add other negative testcases for unsafe prog
> > types. Right now we're just testing tracing progs.
>
> I'm always for less macro usage, if possible :)
>
> For the use case you are describing I'd just add static subprog that
> exercises all the kfuncs of interest, and then call this subprog from
> all the (explicitly defined) main entry program of desired program
> types
Will do for v2. Thanks!
On Thu, Apr 4, 2024 at 3:30 PM David Vernet <[email protected]> wrote:
>
> On Thu, Apr 04, 2024 at 03:16:56PM -0700, Andrii Nakryiko wrote:
> > On Thu, Apr 4, 2024 at 9:33 AM David Vernet <[email protected]> wrote:
> > >
> > > On Thu, Apr 04, 2024 at 09:04:19AM -0700, Yonghong Song wrote:
> > > >
> > > > On 4/3/24 6:03 PM, David Vernet wrote:
> > > > > Now that we can call some kfuncs from BPF_PROG_TYPE_SYSCALL progs, let's
> > > > > add some selftests that verify as much. As a bonus, let's also verify
> > > > > that we can't call the progs from raw tracepoints.
> > > > >
> > > > > Signed-off-by: David Vernet <[email protected]>
> > > >
> > > > Ack with some comments below.
> > > >
> > > > Acked-by: Yonghong Song <[email protected]>
> > >
> > > Thanks for the review. It looks like accidentally replied directly to
> > > me, so I'll re-add the missing cc's.
> > >
> >
> > And dropped bpf@vger :) adding back
>
> *facepalm*, thanks!
>
> >
> > > >
> > > > > ---
> > > > > .../selftests/bpf/prog_tests/cgrp_kfunc.c | 1 +
> > > > > .../selftests/bpf/prog_tests/task_kfunc.c | 1 +
> > > > > .../selftests/bpf/progs/cgrp_kfunc_common.h | 21 +++++++++++++++++++
> > > > > .../selftests/bpf/progs/cgrp_kfunc_failure.c | 4 ++++
> > > > > .../selftests/bpf/progs/cgrp_kfunc_success.c | 4 ++++
> > > > > .../selftests/bpf/progs/cpumask_common.h | 19 +++++++++++++++++
> > > > > .../selftests/bpf/progs/cpumask_failure.c | 4 ++++
> > > > > .../selftests/bpf/progs/cpumask_success.c | 3 +++
> > > > > .../selftests/bpf/progs/task_kfunc_common.h | 18 ++++++++++++++++
> > > > > .../selftests/bpf/progs/task_kfunc_failure.c | 4 ++++
> > > > > .../selftests/bpf/progs/task_kfunc_success.c | 4 ++++
> > > > > 11 files changed, 83 insertions(+)
> > > > >
> > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/cgrp_kfunc.c b/tools/testing/selftests/bpf/prog_tests/cgrp_kfunc.c
> > > > > index adda85f97058..73f0ec4f4eb7 100644
> > > > > --- a/tools/testing/selftests/bpf/prog_tests/cgrp_kfunc.c
> > > > > +++ b/tools/testing/selftests/bpf/prog_tests/cgrp_kfunc.c
> > > > > @@ -102,6 +102,7 @@ void test_cgrp_kfunc(void)
> > > > > run_success_test(success_tests[i]);
> > > > > }
> > > > > + RUN_TESTS(cgrp_kfunc_success);
> > > > > RUN_TESTS(cgrp_kfunc_failure);
> > > > > cleanup:
> > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/task_kfunc.c b/tools/testing/selftests/bpf/prog_tests/task_kfunc.c
> > > > > index d4579f735398..3db4c8601b70 100644
> > > > > --- a/tools/testing/selftests/bpf/prog_tests/task_kfunc.c
> > > > > +++ b/tools/testing/selftests/bpf/prog_tests/task_kfunc.c
> > > > > @@ -94,5 +94,6 @@ void test_task_kfunc(void)
> > > > > run_success_test(success_tests[i]);
> > > > > }
> > > > > + RUN_TESTS(task_kfunc_success);
> > > > > RUN_TESTS(task_kfunc_failure);
> > > > > }
> > > >
> > > > The above RUN_TESTS(cgrp_kfunc_success) and RUN_TESTS(task_kfunc_success)
> > > > will do duplicate work for *existing* bpf programs in their respective
> > > > files. I think we still prefer to have cgrp_kfunc_success tests
> > > > in cgrp_kfunc.c to make it easy to cross check. But in order to
> > > > remove duplicate work, one option is to make other non-RUN_TESTS
> > > > programs in those files not auto-loaded and their corresponding
> > > > prog_tests/*.c file need to explicitly enable loading the problem.
> > >
> > > Good point, and yes I agree with that approach of not auto-loading
> > > non-RUN_TESTS programs. Considering that we have a __success BTF tag to
> > > say, "this prog should successfully load", it seems odd that we'd also
> > > automatically load and validate progs that _didn't_ specify that tag as
> > > well. At that point, I'm not sure what value the tag is bringing. Also,
> >
> > Just more explicitness (if desired). Normally __success would be
> > augmented by __msg() or __retval(). I'd feel uncomfortable just
>
> But __success really has no actual purpose, right? Isn't it identical to
> if it's just left off? You don't need __success to specify __msg() or
> __retval() right?
right, it's just a more explicit documentation-like annotation, if you will
>
> > silently skipping programs that are not marked with __success, as it
> > would be too easy to accidentally forget to add it and not know that
> > the BPF program is not tested.
> >
> > I'd say that RUN_TESTS-based programs should be kept separate from any
> > other BPF programs that have a custom user-space testing part, though.
>
> IF we do go this way, maybe just a __skip or something tag would be
> sufficient?
if we go this way we wouldn't need __skip, but if we do not go, then
sure, why not. But in general, __skip makes sense either way, I guess,
I have no problem with it.
>
> > About the patch itself. I don't really see much point in adding
> > *_KFUNC_LOAD_TEST macros. They are used once or twice in total, while
> > obscuring *what* is actually being tested. Unless you expect to add 5+
> > more copies of them, I'd just inline them explicitly.
>
> It's not really important what's in the actual prog though -- the point
> is that we're verifying we can invoke some kfuncs in a certain prog
> type. But yes, it does obscure what's there, and I'm fine with
> copy-pasting them if that's your preference. The reason I went with a
> macro was to make it easy for us to quickly test new prog types as we
> add support for them, or to add other negative testcases for unsafe prog
> types. Right now we're just testing tracing progs.
I'm always for less macro usage, if possible :)
For the use case you are describing I'd just add static subprog that
exercises all the kfuncs of interest, and then call this subprog from
all the (explicitly defined) main entry program of desired program
types
>
> >
> > > that was the expected behavior before RUN_TESTS() was introduced, so it
> > > hopefully shouldn't cause much if any churn.
> > >
> > > > Maybe the current patch is okay even with duplicated work as it
> > > > should not take much time to verify those tiny problems.
> > >
> > > IMO it should be fine for now as the overhead for validating and loading
> > > these progs is low, but it'd definitely be good to address this problem
> > > in a follow-up. I don't think it should take too much effort -- AFAICT
> > > we'd just have to mark a test spec as invalid if it didn't have any BTF
> > > test tags. Ideally I'd like to separate that from this patch set, but I
> > > can do it here if folks want.
> > >
> > > Thanks,
> > > David