On Fri, Apr 07, 2023 at 05:12:24AM +0200, Kumar Kartikeya Dwivedi wrote:
> 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).
or we simply don't allow calling subprogs that can throw while holding resources.
> 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.
exactly. +1 to all of the above. the progs shouldn't be paying run-time cost
for this 0.0001% case.
