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