| Message ID | 20230809114116.3216687-1-memxor@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Exceptions - 1/2 | expand |
On Wed, 9 Aug 2023 at 17:11, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > [...] > > Known issues > ------------ > > * Just asm volatile ("call bpf_throw" :::) does not emit DATASEC .ksyms > for bpf_throw, there needs to be explicit call in C for clang to emit > the DATASEC info in BTF, leading to errors during compilation. > Hi Yonghong, I'd like to ask you about this issue to figure out whether this is something worth fixing in clang or not. It pops up in programs which only use bpf_assert macros (which emit the call to bpf_throw using inline assembly) and not bpf_throw kfunc directly. I believe in case we emit a call bpf_throw instruction, the BPF backend code will not see any DWARF debug info for the respective symbol, so it will also not be able to convert it and emit anything to .BTF section in case no direct call without asm volatile is present. Therefore my guess is that this isn't something that can be fixed in clang/LLVM. There are also options like the one below to work around it. if ((volatile int){0}) bpf_throw(); asm volatile ("call bpf_throw");
> On Wed, 9 Aug 2023 at 17:11, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: >> >> [...] >> >> Known issues >> ------------ >> >> * Just asm volatile ("call bpf_throw" :::) does not emit DATASEC .ksyms >> for bpf_throw, there needs to be explicit call in C for clang to emit >> the DATASEC info in BTF, leading to errors during compilation. >> > > Hi Yonghong, I'd like to ask you about this issue to figure out > whether this is something worth fixing in clang or not. > It pops up in programs which only use bpf_assert macros (which emit > the call to bpf_throw using inline assembly) and not bpf_throw kfunc > directly. > > I believe in case we emit a call bpf_throw instruction, the BPF > backend code will not see any DWARF debug info for the respective > symbol, so it will also not be able to convert it and emit anything to > .BTF section in case no direct call without asm volatile is present. > Therefore my guess is that this isn't something that can be fixed in > clang/LLVM. Besides, please keep in mind that GCC doens't have an integrated assembler, and therefore relying on clang's understanding on the instructions in inline assembly is something to avoid. > There are also options like the one below to work around it. > if ((volatile int){0}) bpf_throw(); > asm volatile ("call bpf_throw"); I can confirm the above results in a BTF entry for bpf_throw with bpf-unknown-none-gcc -gbtf.
On 8/22/23 3:07 PM, Jose E. Marchesi wrote: > >> On Wed, 9 Aug 2023 at 17:11, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: >>> >>> [...] >>> >>> Known issues >>> ------------ >>> >>> * Just asm volatile ("call bpf_throw" :::) does not emit DATASEC .ksyms >>> for bpf_throw, there needs to be explicit call in C for clang to emit >>> the DATASEC info in BTF, leading to errors during compilation. >>> >> >> Hi Yonghong, I'd like to ask you about this issue to figure out >> whether this is something worth fixing in clang or not. >> It pops up in programs which only use bpf_assert macros (which emit >> the call to bpf_throw using inline assembly) and not bpf_throw kfunc >> directly. >> >> I believe in case we emit a call bpf_throw instruction, the BPF >> backend code will not see any DWARF debug info for the respective >> symbol, so it will also not be able to convert it and emit anything to >> .BTF section in case no direct call without asm volatile is present. >> Therefore my guess is that this isn't something that can be fixed in >> clang/LLVM. > > Besides, please keep in mind that GCC doens't have an integrated > assembler, and therefore relying on clang's understanding on the > instructions in inline assembly is something to avoid. > >> There are also options like the one below to work around it. >> if ((volatile int){0}) bpf_throw(); >> asm volatile ("call bpf_throw"); > > I can confirm the above results in a BTF entry for bpf_throw with > bpf-unknown-none-gcc -gbtf. Kumar, you are correct. For clang, symbols inside 'asm volatile' statement or generally inside any asm code (e.g., kernel .s files) won't generate an entry in dwarf. The if ((volatile int){0}) bpf_throw(); will force a dwarf, hence btf, entry. The unfortunately thing is the above code will generate redundant code like 0000000000000000 <foo>: 0: b7 01 00 00 00 00 00 00 r1 = 0x0 1: 63 1a fc ff 00 00 00 00 *(u32 *)(r10 - 0x4) = r1 2: 61 a1 fc ff 00 00 00 00 r1 = *(u32 *)(r10 - 0x4) 3: 15 01 01 00 00 00 00 00 if r1 == 0x0 goto +0x1 <LBB0_2> 4: 85 10 00 00 ff ff ff ff call -0x1 0000000000000028 <LBB0_2>: 5: 85 10 00 00 ff ff ff ff call -0x1 6: b7 00 00 00 00 00 00 00 r0 = 0x0 7: 95 00 00 00 00 00 00 00 exit I am curious why in bpf_assert macro bpf_throw() kfunc cannot be used?
On Wed, 23 Aug 2023 at 04:09, Yonghong Song <yonghong.song@linux.dev> wrote: > > > > On 8/22/23 3:07 PM, Jose E. Marchesi wrote: > > > >> On Wed, 9 Aug 2023 at 17:11, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > >>> > >>> [...] > >>> > >>> Known issues > >>> ------------ > >>> > >>> * Just asm volatile ("call bpf_throw" :::) does not emit DATASEC .ksyms > >>> for bpf_throw, there needs to be explicit call in C for clang to emit > >>> the DATASEC info in BTF, leading to errors during compilation. > >>> > >> > >> Hi Yonghong, I'd like to ask you about this issue to figure out > >> whether this is something worth fixing in clang or not. > >> It pops up in programs which only use bpf_assert macros (which emit > >> the call to bpf_throw using inline assembly) and not bpf_throw kfunc > >> directly. > >> > >> I believe in case we emit a call bpf_throw instruction, the BPF > >> backend code will not see any DWARF debug info for the respective > >> symbol, so it will also not be able to convert it and emit anything to > >> .BTF section in case no direct call without asm volatile is present. > >> Therefore my guess is that this isn't something that can be fixed in > >> clang/LLVM. > > > > Besides, please keep in mind that GCC doens't have an integrated > > assembler, and therefore relying on clang's understanding on the > > instructions in inline assembly is something to avoid. > > > >> There are also options like the one below to work around it. > >> if ((volatile int){0}) bpf_throw(); > >> asm volatile ("call bpf_throw"); > > > > I can confirm the above results in a BTF entry for bpf_throw with > > bpf-unknown-none-gcc -gbtf. > > Kumar, you are correct. > For clang, symbols inside 'asm volatile' statement or generally > inside any asm code (e.g., kernel .s files) won't generate an entry > in dwarf. The > if ((volatile int){0}) bpf_throw(); > will force a dwarf, hence btf, entry. > > The unfortunately thing is the above code will generate redundant code > like > 0000000000000000 <foo>: > 0: b7 01 00 00 00 00 00 00 r1 = 0x0 > 1: 63 1a fc ff 00 00 00 00 *(u32 *)(r10 - 0x4) = r1 > 2: 61 a1 fc ff 00 00 00 00 r1 = *(u32 *)(r10 - 0x4) > 3: 15 01 01 00 00 00 00 00 if r1 == 0x0 goto +0x1 <LBB0_2> > 4: 85 10 00 00 ff ff ff ff call -0x1 > > 0000000000000028 <LBB0_2>: > 5: 85 10 00 00 ff ff ff ff call -0x1 > 6: b7 00 00 00 00 00 00 00 r0 = 0x0 > 7: 95 00 00 00 00 00 00 00 exit > Yes, I am relying on the verifier to eliminate dead code later, but it is obviously a hack. > I am curious why in bpf_assert macro bpf_throw() kfunc cannot > be used? The reason was to force the compiler to emit a specific branch for the assertion check without being influenced by compiler optimizations, and tying comparison to the register holding a value being tested in assertion. Secondly we also enforce that the first argument is in a register and the second a constant, so as to apply the verifier bounds gained after comparison op to the original register. I am aware (though correct me if this is wrong) that the compiler can select a different register for the input operand of the asm constraint, but find_equal_id_scalars should still do correct propagation. This is partly why I disabled usage of this macro for variable widths < 64-bit, because then the compiler sometimes performs shifts etc. for integer promotion implicitly, sometimes destroying whatever information we gained through an assertion check. Depending on the signedness of the variable, we emit the signed/unsigned comparison. I suppose we could switch to the ' if (!(LHS <op> RHS)) bpf_throw(); ' sequence in C, force volatile load for LHS and __builtin_constant_p for RHS to get the same behavior. Emitting these redundant checks is definitely a bit weird just to emit BTF.
On Wed, 23 Aug 2023 at 03:37, Jose E. Marchesi <jose.marchesi@oracle.com> wrote: > > > > On Wed, 9 Aug 2023 at 17:11, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > >> > >> [...] > >> > >> Known issues > >> ------------ > >> > >> * Just asm volatile ("call bpf_throw" :::) does not emit DATASEC .ksyms > >> for bpf_throw, there needs to be explicit call in C for clang to emit > >> the DATASEC info in BTF, leading to errors during compilation. > >> > > > > Hi Yonghong, I'd like to ask you about this issue to figure out > > whether this is something worth fixing in clang or not. > > It pops up in programs which only use bpf_assert macros (which emit > > the call to bpf_throw using inline assembly) and not bpf_throw kfunc > > directly. > > > > I believe in case we emit a call bpf_throw instruction, the BPF > > backend code will not see any DWARF debug info for the respective > > symbol, so it will also not be able to convert it and emit anything to > > .BTF section in case no direct call without asm volatile is present. > > Therefore my guess is that this isn't something that can be fixed in > > clang/LLVM. > > Besides, please keep in mind that GCC doens't have an integrated > assembler, and therefore relying on clang's understanding on the > instructions in inline assembly is something to avoid. > Thank you for reminding me that. I will be more careful about this. We certainly cannot rely on clang-specific behavior for this. > > There are also options like the one below to work around it. > > if ((volatile int){0}) bpf_throw(); > > asm volatile ("call bpf_throw"); > > I can confirm the above results in a BTF entry for bpf_throw with > bpf-unknown-none-gcc -gbtf. Thanks for confirming.
On Tue, Aug 22, 2023 at 3:54 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > > I suppose we could switch to the ' if (!(LHS <op> RHS)) bpf_throw(); ' > sequence in C, force volatile load for LHS and __builtin_constant_p > for RHS to get the same behavior. Emitting these redundant checks is > definitely a bit weird just to emit BTF. I guess we can try #define bpf_assert(LHS, OP, RHS) if (!(LHS OP RHS)) bpf_throw(); with barrier_var(LHS) and __builtin_constant_p(RHS) and keep things completely in C, but there is no guarantee that the compiler will not convert == to !=, swap lhs and rhs, etc. Maybe we can have both asm and C style macros, then recommend C to start and switch to asm if things are dodgy. Feels like dangerous ambiguity.
On Tue, Aug 22, 2023 at 4:06 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Tue, Aug 22, 2023 at 3:54 PM Kumar Kartikeya Dwivedi > <memxor@gmail.com> wrote: > > > > > > I suppose we could switch to the ' if (!(LHS <op> RHS)) bpf_throw(); ' > > sequence in C, force volatile load for LHS and __builtin_constant_p > > for RHS to get the same behavior. Emitting these redundant checks is > > definitely a bit weird just to emit BTF. > > I guess we can try > #define bpf_assert(LHS, OP, RHS) if (!(LHS OP RHS)) bpf_throw(); > with barrier_var(LHS) and __builtin_constant_p(RHS) and > keep things completely in C, > but there is no guarantee that the compiler will not convert == to !=, > swap lhs and rhs, etc. > Maybe we can have both asm and C style macros, then recommend C to start > and switch to asm if things are dodgy. > Feels like dangerous ambiguity. This seems similar to the issue I had with __attribute__((cleanup(some_kfunc)))) not emitting BTF info for that some_kfunc? See bpf_for_each(), seems like just adding `(void)bpf_iter_##type##_destroy` makes Clang emit BTF info. It would be nice to have this fixed for cleanup() attribute and asm, of course. But this is a simple work around.
On Sat, 26 Aug 2023 at 00:25, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Tue, Aug 22, 2023 at 4:06 PM Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: > > > > On Tue, Aug 22, 2023 at 3:54 PM Kumar Kartikeya Dwivedi > > <memxor@gmail.com> wrote: > > > > > > > > > I suppose we could switch to the ' if (!(LHS <op> RHS)) bpf_throw(); ' > > > sequence in C, force volatile load for LHS and __builtin_constant_p > > > for RHS to get the same behavior. Emitting these redundant checks is > > > definitely a bit weird just to emit BTF. > > > > I guess we can try > > #define bpf_assert(LHS, OP, RHS) if (!(LHS OP RHS)) bpf_throw(); > > with barrier_var(LHS) and __builtin_constant_p(RHS) and > > keep things completely in C, > > but there is no guarantee that the compiler will not convert == to !=, > > swap lhs and rhs, etc. > > Maybe we can have both asm and C style macros, then recommend C to start > > and switch to asm if things are dodgy. > > Feels like dangerous ambiguity. > > This seems similar to the issue I had with > __attribute__((cleanup(some_kfunc)))) not emitting BTF info for that > some_kfunc? See bpf_for_each(), seems like just adding > `(void)bpf_iter_##type##_destroy` makes Clang emit BTF info. > > It would be nice to have this fixed for cleanup() attribute and asm, > of course. But this is a simple work around. Good to know, this is cleaner than my solution. But I am planning to switch to the direct C approach, so it should not be needed anymore.
This series implements the _first_ part of the runtime and verifier support needed to enable BPF exceptions. Exceptions thrown from programs are processed as an immediate exit from the program, which unwinds all the active stack frames until the main stack frame, and returns to the BPF program's caller. The ability to perform this unwinding safely allows the program to test conditions that are always true at runtime but which the verifier has no visibility into. Thus, it also reduces verification effort by safely terminating redundant paths that can be taken within a program. The patches to perform runtime resource cleanup during the frame-by-frame unwinding will be posted as a follow-up to this set. It must be noted that exceptions are not an error handling mechanism for unlikely runtime conditions, but a way to safely terminate the execution of a program in presence of conditions that should never occur at runtime. They are meant to serve higher-level primitives such as program assertions. The following kfuncs and macros are introduced: Assertion macros are also introduced, please see patch 13 for their documentation. /* Description * Throw a BPF exception from the program, immediately terminating its * execution and unwinding the stack. The supplied 'cookie' parameter * will be the return value of the program when an exception is thrown, * and the default exception callback is used. Otherwise, if an exception * callback is set using the '__exception_cb(callback)' declaration tag * on the main program, the 'cookie' parameter will be the callback's only * input argument. * * Thus, in case of default exception callback, 'cookie' is subjected to * constraints on the program's return value (as with R0 on exit). * Otherwise, the return value of the marked exception callback will be * subjected to the same checks. * * Note that throwing an exception with lingering resources (locks, * references, etc.) will lead to a verification error. * * Note that callbacks *cannot* call this helper. * Returns * Never. * Throws * An exception with the specified 'cookie' value. */ extern void bpf_throw(u64 cookie) __ksym; /* This macro must be used to mark the exception callback corresponding to the * main program. For example: * * int exception_cb(u64 cookie) { * return cookie; * } * * SEC("tc") * __exception_cb(exception_cb) * int main_prog(struct __sk_buff *ctx) { * ... * return TC_ACT_OK; * } * * Here, exception callback for the main program will be 'exception_cb'. Note * that this attribute can only be used once, and multiple exception callbacks * specified for the main program will lead to verification error. */ \#define __exception_cb(name) __attribute__((btf_decl_tag("exception_callback:" #name))) As such, a program can only install an exception handler once for the lifetime of a BPF program, and this handler cannot be changed at runtime. The purpose of the handler is to simply interpret the cookie value supplied by the bpf_throw call, and execute user-defined logic corresponding to it. The primary purpose of allowing a handler is to control the return value of the program. The default handler returns the cookie value passed to bpf_throw when an exception is thrown. Fixing the handler for the lifetime of the program eliminates tricky and expensive handling in case of runtime changes of the handler callback when programs begin to nest, where it becomes more complex to save and restore the active handler at runtime. This version of offline unwinding based BPF exceptions is truly zero overhead, with the exception of generation of a default callback which contains a few instructions to return a default return value (0) when no exception callback is supplied by the user. Callbacks are disallowed from throwing BPF exceptions for now, since such exceptions need to cross the callback helper boundary (and therefore must care about unwinding kernel state), however it is possible to lift this restriction in the future follow-up. Exceptions terminate propogating at program boundaries, hence both BPF_PROG_TYPE_EXT and tail call targets return to their caller context the return value of the exception callback, in the event that they throw an exception. Thus, exceptions do not cross extension or tail call boundary. However, this is mostly an implementation choice, and can be changed to suit more user-friendly semantics. Known issues ------------ * Just asm volatile ("call bpf_throw" :::) does not emit DATASEC .ksyms for bpf_throw, there needs to be explicit call in C for clang to emit the DATASEC info in BTF, leading to errors during compilation. Changelog: ---------- v1 -> v2 v1: https://lore.kernel.org/bpf/20230713023232.1411523-1-memxor@gmail.com * Address all comments from Alexei. * Fix a few bugs and corner cases in the implementations found during testing. Also add new selftests for these cases. * Reinstate patch to consider ksym.end part of the program (but reworked to cover other corner cases). * Implement new style of tagging exception callbacks, add libbpf support for the new declaration tag. * Limit support to 64-bit integer types for assertion macros. The compiler ends up performing shifts or bitwise and operations when finally making use of the value, which defeats the purpose of the macro. On noalu32 mode, the shifts may also happen before use, hurting reliability. * Comprehensively test assertion macros and their side effects on the verifier state, register bounds, etc. * Fix a KASAN false positive warning. RFC v1 -> v1 RFC v1: https://lore.kernel.org/bpf/20230405004239.1375399-1-memxor@gmail.com * Completely rework the unwinding infrastructure to use offline unwinding support. * Remove the runtime exception state and program rewriting code. * Make bpf_set_exception_callback idempotent to avoid vexing synchronization and state clobbering issues in presence of program nesting. * Disable bpf_throw within callback functions, for now. * Allow bpf_throw in tail call programs and extension programs, removing limitations of rewrite based unwinding. * Expand selftests. Kumar Kartikeya Dwivedi (14): arch/x86: Implement arch_bpf_stack_walk bpf: Implement support for adding hidden subprogs bpf: Implement BPF exceptions bpf: Refactor check_btf_func and split into two phases bpf: Add support for custom exception callbacks bpf: Perform CFG walk for exception callback bpf: Treat first argument as return value for bpf_throw bpf: Prevent KASAN false positive with bpf_throw bpf: Detect IP == ksym.end as part of BPF program bpf: Disallow extensions to exception callbacks bpf: Fix kfunc callback register type handling libbpf: Add support for custom exception callbacks selftests/bpf: Add BPF assertion macros selftests/bpf: Add tests for BPF exceptions arch/x86/net/bpf_jit_comp.c | 118 ++++- include/linux/bpf.h | 8 +- include/linux/bpf_verifier.h | 8 +- include/linux/filter.h | 8 + include/linux/kasan.h | 2 + kernel/bpf/btf.c | 29 +- kernel/bpf/core.c | 29 +- kernel/bpf/helpers.c | 45 ++ kernel/bpf/syscall.c | 2 +- kernel/bpf/verifier.c | 455 +++++++++++++++--- tools/lib/bpf/libbpf.c | 166 ++++++- tools/testing/selftests/bpf/DENYLIST.aarch64 | 1 + tools/testing/selftests/bpf/DENYLIST.s390x | 1 + .../testing/selftests/bpf/bpf_experimental.h | 287 +++++++++++ .../selftests/bpf/prog_tests/exceptions.c | 324 +++++++++++++ .../testing/selftests/bpf/progs/exceptions.c | 368 ++++++++++++++ .../selftests/bpf/progs/exceptions_assert.c | 135 ++++++ .../selftests/bpf/progs/exceptions_ext.c | 59 +++ .../selftests/bpf/progs/exceptions_fail.c | 347 +++++++++++++ 19 files changed, 2271 insertions(+), 121 deletions(-) create mode 100644 tools/testing/selftests/bpf/prog_tests/exceptions.c create mode 100644 tools/testing/selftests/bpf/progs/exceptions.c create mode 100644 tools/testing/selftests/bpf/progs/exceptions_assert.c create mode 100644 tools/testing/selftests/bpf/progs/exceptions_ext.c create mode 100644 tools/testing/selftests/bpf/progs/exceptions_fail.c base-commit: 2adbb7637fd1fcec93f4680ddb5ddbbd1a91aefb