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(+)
--
2.42.1
Registering generic_kfunc_set with XDP programs enables some of the
newer BPF features inside XDP -- namely tree based data structures and
BPF exceptions.
The current motivation for this commit is to enable assertions inside
XDP bpf progs. Assertions are a standard and useful tool to encode
intent.
Signed-off-by: Daniel Xu <[email protected]>
---
kernel/bpf/helpers.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index b3be5742d6f1..b0b485126a76 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -2630,6 +2630,7 @@ static int __init kfunc_init(void)
ret = register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, &generic_kfunc_set);
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_id_dtor_kfuncs(generic_dtors,
ARRAY_SIZE(generic_dtors),
--
2.42.1
Add some positive and negative test cases that exercise the "callback"
semantics.
Signed-off-by: Daniel Xu <[email protected]>
---
.../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";
--
2.42.1
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 <[email protected]>
---
.../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
--
2.42.1
On Thu, Dec 14, 2023 at 2:56 PM Daniel Xu <[email protected]> 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 <[email protected]>
> ---
> .../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
<[email protected]> wrote:
>
> On Thu, Dec 14, 2023 at 2:56 PM Daniel Xu <[email protected]> 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 <[email protected]>
> > ---
> > .../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
}
Hello:
This series was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <[email protected]>:
On Thu, 14 Dec 2023 15:56:24 -0700 you wrote:
> 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.
>
> [...]
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!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
On Thu, Dec 14, 2023 at 7:10 PM Alexei Starovoitov
<[email protected]> 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.