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