On Fri, Apr 07, 2023 at 04:30:07AM CEST, Alexei Starovoitov wrote:
> On Fri, Apr 07, 2023 at 02:42:14AM +0200, Kumar Kartikeya Dwivedi wrote:
> > On Thu, Apr 06, 2023 at 04:38:09AM CEST, Alexei Starovoitov wrote:
> > > On Wed, Apr 05, 2023 at 02:42:39AM +0200, Kumar Kartikeya Dwivedi wrote:
> > > > +static __noinline int throwing_subprog(struct __sk_buff *ctx)
> > > > +{
> > > > + if (ctx)
> > > > + bpf_throw();
> > > > + return 0;
> > > > +}
> > > > +
> > > > +__noinline int global_subprog(struct __sk_buff *ctx)
> > > > +{
> > > > + return subprog(ctx) + 1;
> > > > +}
> > > > +
> > > > +__noinline int throwing_global_subprog(struct __sk_buff *ctx)
> > > > +{
> > > > + if (ctx)
> > > > + bpf_throw();
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static __noinline int exception_cb(void)
> > > > +{
> > > > + return 16;
> > > > +}
> > > > +
> > > > +SEC("tc")
> > > > +int exception_throw_subprog(struct __sk_buff *ctx)
> > > > +{
> > > > + volatile int i;
> > > > +
> > > > + exception_cb();
> > > > + bpf_set_exception_callback(exception_cb);
> > > > + i = subprog(ctx);
> > > > + i += global_subprog(ctx) - 1;
> > > > + if (!i)
> > > > + return throwing_global_subprog(ctx);
> > > > + else
> > > > + return throwing_subprog(ctx);
> > > > + bpf_throw();
> > > > + return 0;
> > > > +}
> > > > +
> > > > +__noinline int throwing_gfunc(volatile int i)
> > > > +{
> > > > + bpf_assert_eq(i, 0);
> > > > + return 1;
> > > > +}
> > > > +
> > > > +__noinline static int throwing_func(volatile int i)
> > > > +{
> > > > + bpf_assert_lt(i, 1);
> > > > + return 1;
> > > > +}
> > >
> > > exception_cb() has no way of knowning which assert statement threw the exception.
> > > How about extending a macro:
> > > bpf_assert_eq(i, 0, MY_INT_ERR);
> > > or
> > > bpf_assert_eq(i, 0) {bpf_throw(MY_INT_ERR);}
> > >
> > > bpf_throw can store it in prog->aux->exception pass the address to cb.
> > >
> >
> > I agree and will add passing of a value that gets passed to the callback
> > (probably just set it in the exception state), but I don't think prog->aux will
> > work, see previous mails.
> >
> > > Also I think we shouldn't complicate the verifier with auto release of resources.
> > > If the user really wants to assert when spin_lock is held it should be user's
> > > job to specify what resources should be released.
> > > Can we make it look like:
> > >
> > > bpf_spin_lock(&lock);
> > > bpf_assert_eq(i, 0) {
> > > bpf_spin_unlock(&lock);
> > > bpf_throw(MY_INT_ERR);
> > > }
> >
> > Do you mean just locks or all resources? Then it kind of undermines the point of
> > having something like bpf_throw IMO. Since it's easy to insert code from the
> > point of throw but it's not possible to do the same in callers (unless we add a
> > way to 'catch' throws), so it only works for some particular cases where callers
> > don't hold references (or in the main subprog).
>
> That's a good point. This approach will force caller of functions that can
> throw not hold any referenced objects (spin_locks, sk_lookup, obj_new, etc)
> That indeed might be too restrictive.
> It would be great if we could come up with a way for bpf prog to release resources
> explicitly though. I'm not a fan of magic release of spin locks.
>
I think to realize that, we need a way to 'catch' thrown exceptions in the
caller. The user will write a block of code that handles release of resources in
the current scope and resume unwinding (implicitly by calling into some
intrinsics). When we discussed this earlier, you rightly mentioned problems
associated with introducing control flow into the program not seen by the
compiler (unlike try/catch in something like C++ which has clear semantics).
But to take a step back, and about what you've said below:
> bpf_assert is not analogous to C++ exceptions. It shouldn't be seen a mechanism
> to simplify control flow and error handling.
When programs get explicit control and handle the 'exceptional' condition, it
begins to mimic more of an alternative 'slow path' of the program when it
encounters an error, and starts resembling an alternative error handling
mechanism pretty much like C++ exceptions.
I think that was not the goal here, and the automatic release of resources is
simply supposed to happen to ensure that the kernel's resource safety is not
violated when a program is aborting. An assertion's should never occur at
runtime 99.9999% of the time, but when it does, the BPF runtime emits code to
ensure that the aborting program does not leave the kernel in an inconsistent
state (memory leaks, held locks that other programs can never take again, etc.).
So this will involve clean up of every resource it had acquired when it threw.
If we cannot ensure that we can safely release resources (e.g. throwing in NMI
context where a kptr_xchg'd pointer cannot be freed), we will bail during
verification.
> > There are also other ways to go about this whole thing, like having the compiler
> > emit calls to instrinsics which the BPF runtime provides (or have the call
> > configurable through compiler switches), and it already emits the landing pad
> > code to release stuff and we simply receive the table of pads indexed by each
> > throwing instruction, perform necessary checks to ensure everything is actually
> > released correctly when control flow goes through them (e.g. when exploring
> > multiple paths through the same instruction), and unwind frame by frame. That
> > reduces the burden on both the verifier and user, but then it would be probably
> > need to be BPF C++, or have to be a new language extension for BPF C. E.g. there
> > was something about defer, panic, recover etc. in wg14
> > https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2542.pdf . Having the compiler
> > do it is also probably easier if we want 'catch' style handlers.
>
> Interesting idea. __attribute__((cleanup(..))) may be handy, but that's a ton of work.
> I'm in a camp of developers who enforce -fno-exceptions in C++ projects.
> bpf_assert is not analogous to C++ exceptions. It shouldn't be seen a mechanism
> to simplify control flow and error handling.
