[PATCH bpf-next 0/3] Various BPF exception improvements

[PATCH bpf-next 0/3] Various BPF exception improvements

[Daniel Xu]

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

[PATCH bpf-next 1/3] bpf: xdp: Register generic_kfunc_set with XDP programs

[Daniel Xu]

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 <dxu@dxuuu.xyz>
---
 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

[PATCH bpf-next 2/3] bpf: selftests: Add bpf_assert_if() and bpf_assert_with_if() macros

[Daniel Xu]

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
-- 
2.42.1

[PATCH bpf-next 3/3] bpf: selftests: Test bpf_assert_if() and bpf_assert_with_if()

[Daniel Xu]

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";
-- 
2.42.1

Re: [PATCH bpf-next 2/3] bpf: selftests: Add bpf_assert_if() and bpf_assert_with_if() macros

[Alexei Starovoitov]

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(...); }

Re: [PATCH bpf-next 2/3] bpf: selftests: Add bpf_assert_if() and bpf_assert_with_if() macros

[Alexei Starovoitov]

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
}

Re: [PATCH bpf-next 0/3] Various BPF exception improvements

[patchwork-bot+netdevbpf]

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:
> 
> 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

Re: [PATCH bpf-next 2/3] bpf: selftests: Add bpf_assert_if() and bpf_assert_with_if() macros

[Alexei Starovoitov]

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.