Two small improves to BPF exceptions in this patchset:
1. Allow throwing exceptions in XDP progs 2. Add some macros to help release references before throwing exceptions
Note the macros are intended to be temporary, at least until BPF exception infra is able to automatically release acquired resources.
Daniel Xu (3): bpf: xdp: Register generic_kfunc_set with XDP programs bpf: selftests: Add bpf_assert_if() and bpf_assert_with_if() macros bpf: selftests: Test bpf_assert_if() and bpf_assert_with_if()
kernel/bpf/helpers.c | 1 + .../testing/selftests/bpf/bpf_experimental.h | 22 +++++++ .../selftests/bpf/prog_tests/exceptions.c | 5 ++ .../testing/selftests/bpf/progs/exceptions.c | 61 +++++++++++++++++++ 4 files changed, 89 insertions(+)
These macros are a temporary stop-gap until bpf exceptions support unwinding acquired entities. Basically these macros act as if they take a callback which only get executed if the assertion fails.
Signed-off-by: Daniel Xu dxu@dxuuu.xyz --- .../testing/selftests/bpf/bpf_experimental.h | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/tools/testing/selftests/bpf/bpf_experimental.h b/tools/testing/selftests/bpf/bpf_experimental.h index 1386baf9ae4a..d63f415bef26 100644 --- a/tools/testing/selftests/bpf/bpf_experimental.h +++ b/tools/testing/selftests/bpf/bpf_experimental.h @@ -263,6 +263,17 @@ extern void bpf_throw(u64 cookie) __ksym; */ #define bpf_assert(cond) if (!(cond)) bpf_throw(0);
+/* Description + * Assert that a conditional expression is true. If false, runs code in the + * body before throwing. + * Returns + * Void. + * Throws + * An exception with the value zero when the assertion fails. + */ +#define bpf_assert_if(cond) \ + for (int ___i = 0, ___j = !!(cond); !(___j) && !___i; bpf_throw(0), ___i++) + /* Description * Assert that a conditional expression is true. * Returns @@ -272,6 +283,17 @@ extern void bpf_throw(u64 cookie) __ksym; */ #define bpf_assert_with(cond, value) if (!(cond)) bpf_throw(value);
+/* Description + * Assert that a conditional expression is true. If false, runs code in the + * body before throwing. + * Returns + * Void. + * Throws + * An exception with the given value when the assertion fails. + */ +#define bpf_assert_with_if(cond, value) \ + for (int ___i = 0, ___j = !!(cond); !(___j) && !___i; bpf_throw(value), ___i++) + /* Description * Assert that LHS is equal to RHS. This statement updates the known value * of LHS during verification. Note that RHS must be a constant value, and
On Thu, Dec 14, 2023 at 2:56 PM Daniel Xu dxu@dxuuu.xyz wrote:
These macros are a temporary stop-gap until bpf exceptions support unwinding acquired entities. Basically these macros act as if they take a callback which only get executed if the assertion fails.
Signed-off-by: Daniel Xu dxu@dxuuu.xyz
.../testing/selftests/bpf/bpf_experimental.h | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/tools/testing/selftests/bpf/bpf_experimental.h b/tools/testing/selftests/bpf/bpf_experimental.h index 1386baf9ae4a..d63f415bef26 100644 --- a/tools/testing/selftests/bpf/bpf_experimental.h +++ b/tools/testing/selftests/bpf/bpf_experimental.h @@ -263,6 +263,17 @@ extern void bpf_throw(u64 cookie) __ksym; */ #define bpf_assert(cond) if (!(cond)) bpf_throw(0);
+/* Description
Assert that a conditional expression is true. If false, runs code in the
body before throwing.
- Returns
Void.
- Throws
An exception with the value zero when the assertion fails.- */
+#define bpf_assert_if(cond) \
for (int ___i = 0, ___j = !!(cond); !(___j) && !___i; bpf_throw(0), ___i++)
Kumar,
Is this approach reliable? I suspect the compiler can still optimize it. I feel it will be annoying to clean up if folks start using it now, since there won't be a drop in replacement. Every such bpf_assert_if() would need to be manually patched.
If 2nd part of exception is far, how about we add an equivalent of __bpf_assert() macroses with conditional ops in asm, but with extra 'asm volatile goto' that can be used to construct release of resources.
bpf_do_assert_eq(var1, 0) { bpf_spin_unlock(...); } bpf_do_assert_lt(var2, 0) { bpf_spin_unlock(...); }
On Thu, Dec 14, 2023 at 6:46 PM Alexei Starovoitov alexei.starovoitov@gmail.com wrote:
On Thu, Dec 14, 2023 at 2:56 PM Daniel Xu dxu@dxuuu.xyz wrote:
These macros are a temporary stop-gap until bpf exceptions support unwinding acquired entities. Basically these macros act as if they take a callback which only get executed if the assertion fails.
Signed-off-by: Daniel Xu dxu@dxuuu.xyz
.../testing/selftests/bpf/bpf_experimental.h | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/tools/testing/selftests/bpf/bpf_experimental.h b/tools/testing/selftests/bpf/bpf_experimental.h index 1386baf9ae4a..d63f415bef26 100644 --- a/tools/testing/selftests/bpf/bpf_experimental.h +++ b/tools/testing/selftests/bpf/bpf_experimental.h @@ -263,6 +263,17 @@ extern void bpf_throw(u64 cookie) __ksym; */ #define bpf_assert(cond) if (!(cond)) bpf_throw(0);
+/* Description
Assert that a conditional expression is true. If false, runs code in the
body before throwing.
- Returns
Void.
- Throws
An exception with the value zero when the assertion fails.- */
+#define bpf_assert_if(cond) \
for (int ___i = 0, ___j = !!(cond); !(___j) && !___i; bpf_throw(0), ___i++)Kumar,
Is this approach reliable? I suspect the compiler can still optimize it. I feel it will be annoying to clean up if folks start using it now, since there won't be a drop in replacement. Every such bpf_assert_if() would need to be manually patched.
If 2nd part of exception is far, how about we add an equivalent of __bpf_assert() macroses with conditional ops in asm, but with extra 'asm volatile goto' that can be used to construct release of resources.
bpf_do_assert_eq(var1, 0) { bpf_spin_unlock(...); } bpf_do_assert_lt(var2, 0) { bpf_spin_unlock(...); }
Just realized that we can go the other way instead.
We can get rid of bpf_assert_eq/ne/... and replace with:
diff --git a/tools/testing/selftests/bpf/bpf_experimental.h b/tools/testing/selftests/bpf/bpf_experimental.h index 1386baf9ae4a..1c500287766d 100644 --- a/tools/testing/selftests/bpf/bpf_experimental.h +++ b/tools/testing/selftests/bpf/bpf_experimental.h @@ -254,6 +254,15 @@ extern void bpf_throw(u64 cookie) __ksym; } \ })
+#define _EQ(LHS, RHS) \ + ({ int var = 1;\ + asm volatile goto("if %[lhs] == %[rhs] goto %l[l_yes]" \ + :: [lhs] "r"(LHS), [rhs] "i"(RHS) :: l_yes);\ + var = 0;\ +l_yes:\ + var;\ + }) + /* Description * Assert that a conditional expression is true. * Returns diff --git a/tools/testing/selftests/bpf/progs/exceptions.c b/tools/testing/selftests/bpf/progs/exceptions.c index 2811ee842b01..1111e852f154 100644 --- a/tools/testing/selftests/bpf/progs/exceptions.c +++ b/tools/testing/selftests/bpf/progs/exceptions.c @@ -203,6 +203,7 @@ __noinline int assert_nz_gfunc(u64 c) volatile u64 cookie = c;
bpf_assert(cookie != 0); + bpf_assert(_EQ(cookie, 2)); return 0; }
we can probably remove bpf_assert_with() and all of the bpf_assert_le|ne|qt|eq|_with()
Users can open code everything: if (!_EQ(foo, bar)) bpf_throw(123);
bpf_assert_if() can work too, but let's call it bpf_do_assert() and use like:
bpf_do_assert(EQ(time, 0)) { // cleanup }
On Thu, Dec 14, 2023 at 7:10 PM Alexei Starovoitov alexei.starovoitov@gmail.com wrote:
Just realized that we can go the other way instead.
We can get rid of bpf_assert_eq/ne/... and replace with:
diff --git a/tools/testing/selftests/bpf/bpf_experimental.h b/tools/testing/selftests/bpf/bpf_experimental.h index 1386baf9ae4a..1c500287766d 100644 --- a/tools/testing/selftests/bpf/bpf_experimental.h +++ b/tools/testing/selftests/bpf/bpf_experimental.h @@ -254,6 +254,15 @@ extern void bpf_throw(u64 cookie) __ksym; } \ })
+#define _EQ(LHS, RHS) \
({ int var = 1;\asm volatile goto("if %[lhs] == %[rhs] goto %l[l_yes]" \:: [lhs] "r"(LHS), [rhs] "i"(RHS) :: l_yes);\var = 0;\+l_yes:\
var;\})
Realized we can do much better. We can take advantage that bpf assembly syntax resembles C and do:
bpf_assert(CMP(cookie, "!=", 0);
and use it as generic "volatile compare" that compiler cannot optimize out:
Replacing: if (foo < bar) ... with if (CMP(foo, "<", bar)) ... when the compare operator should be preserved. I'll try to prototype it soon.
Might go further and use C++ for bpf programs :) Override operator<, opreator==, ... then if (foo < bar) will be in asm code as written in C++ bpf prog.
Add some positive and negative test cases that exercise the "callback" semantics.
Signed-off-by: Daniel Xu dxu@dxuuu.xyz --- .../selftests/bpf/prog_tests/exceptions.c | 5 ++ .../testing/selftests/bpf/progs/exceptions.c | 61 +++++++++++++++++++ 2 files changed, 66 insertions(+)
diff --git a/tools/testing/selftests/bpf/prog_tests/exceptions.c b/tools/testing/selftests/bpf/prog_tests/exceptions.c index 516f4a13013c..a41113d72d0d 100644 --- a/tools/testing/selftests/bpf/prog_tests/exceptions.c +++ b/tools/testing/selftests/bpf/prog_tests/exceptions.c @@ -83,6 +83,11 @@ static void test_exceptions_success(void) RUN_SUCCESS(exception_assert_range_with, 1); RUN_SUCCESS(exception_bad_assert_range, 0); RUN_SUCCESS(exception_bad_assert_range_with, 10); + RUN_SUCCESS(exception_assert_if_body_not_executed, 2); + RUN_SUCCESS(exception_bad_assert_if_body_executed, 1); + RUN_SUCCESS(exception_bad_assert_if_throws, 0); + RUN_SUCCESS(exception_assert_with_if_body_not_executed, 3); + RUN_SUCCESS(exception_bad_assert_with_if_body_executed, 2);
#define RUN_EXT(load_ret, attach_err, expr, msg, after_link) \ { \ diff --git a/tools/testing/selftests/bpf/progs/exceptions.c b/tools/testing/selftests/bpf/progs/exceptions.c index 2811ee842b01..e61cb794a305 100644 --- a/tools/testing/selftests/bpf/progs/exceptions.c +++ b/tools/testing/selftests/bpf/progs/exceptions.c @@ -365,4 +365,65 @@ int exception_bad_assert_range_with(struct __sk_buff *ctx) return 1; }
+SEC("tc") +int exception_assert_if_body_not_executed(struct __sk_buff *ctx) +{ + u64 time = bpf_ktime_get_ns(); + + bpf_assert_if(time != 0) { + return 1; + } + + return 2; +} + +SEC("tc") +int exception_bad_assert_if_body_executed(struct __sk_buff *ctx) +{ + u64 time = bpf_ktime_get_ns(); + + bpf_assert_if(time == 0) { + return 1; + } + + return 2; +} + +SEC("tc") +int exception_bad_assert_if_throws(struct __sk_buff *ctx) +{ + u64 time = bpf_ktime_get_ns(); + + bpf_assert_if(time == 0) { + } + + return 2; +} + +SEC("tc") +int exception_assert_with_if_body_not_executed(struct __sk_buff *ctx) +{ + u64 time = bpf_ktime_get_ns(); + int ret = 1; + + bpf_assert_with_if(time != 0, ret) { + ret = 2; + } + + return 3; +} + +SEC("tc") +int exception_bad_assert_with_if_body_executed(struct __sk_buff *ctx) +{ + u64 time = bpf_ktime_get_ns(); + int ret = 1; + + bpf_assert_with_if(time == 0, ret) { + ret = 2; + } + + return 3; +} + char _license[] SEC("license") = "GPL";
Hello:
This series was applied to bpf/bpf-next.git (master) by Alexei Starovoitov ast@kernel.org:
On Thu, 14 Dec 2023 15:56:24 -0700 you wrote:
Two small improves to BPF exceptions in this patchset:
- Allow throwing exceptions in XDP progs
- Add some macros to help release references before throwing exceptions
Note the macros are intended to be temporary, at least until BPF exception infra is able to automatically release acquired resources.
[...]
Here is the summary with links: - [bpf-next,1/3] bpf: xdp: Register generic_kfunc_set with XDP programs https://git.kernel.org/bpf/bpf-next/c/7489723c2e26 - [bpf-next,2/3] bpf: selftests: Add bpf_assert_if() and bpf_assert_with_if() macros (no matching commit) - [bpf-next,3/3] bpf: selftests: Test bpf_assert_if() and bpf_assert_with_if() (no matching commit)
You are awesome, thank you!
linux-kselftest-mirror@lists.linaro.org
-
Alexei Starovoitov -
Daniel Xu -
patchwork-bot+netdevbpf＠kernel.org