Re: [PATCH bpf v2 2/2] selftests/bpf: Test r0 bounds after BPF to BPF call with abnormal return

["Arthur Fabre" <afabre@cloudflare.com>]

On Mon Dec 16, 2024 at 8:47 PM CET, Alexei Starovoitov wrote:
> On Mon, Dec 16, 2024 at 10:50 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
> >
> > On Mon, 2024-12-16 at 10:05 -0800, Alexei Starovoitov wrote:
> >
> > [...]
> >
> > > > Thanks for the review! Good point, I'll try to write them in C.
> > > >
> > > > It might not be possible to do them both entirely: clang also doesn't
> > > > know that bpf_tail_call() can return, so it assumes the callee() will
> > > > return a constant r0. It sometimes optimizes branches / loads out
> > > > because of this.
> > >
> > > I wonder whether we should tell llvm that it's similar to longjmp()
> > > with __attribute__((noreturn)) or some other attribute.
> >
> > GCC documents it as follows [1]:
> >
> >   > The noreturn keyword tells the compiler to assume that fatal
> >   > cannot reaturn. It can then optimize without regard to what would
> >   > happen if fatal ever did return. This makes slightly better code.
> >   > More importantly, it helps avoid spurious warnings of
> >   > uninitialized variables.
> >
> > But the bpf_tail_call could return if MAX_TAIL_CALL_CNT limit is exceeded,
> > or programs map index is out of bounds.
> >
> > [1] https://gcc.gnu.org/onlinedocs/gcc-14.2.0/gcc/Common-Function-Attributes.html#index-noreturn-function-attribute
>
> Yeah. noreturn is too heavy.
> attr(returns_twice) is another option, but it will probably
> pessimize the code too much as well.

We could provide a macro like:

#define BPF_TAIL_CALL(ctx, map, index) do { \
    bpf_tail_call(ctx, map, index); \
    int maybe_return; \
    asm volatile("%0 = 0" : "=r"(maybe_return)); \
    if (maybe_return) \
        return maybe_return; \
} while(0)

It correctly tricks clang into thinking it can return early, but the
verifier removes the dead branch so there's no runtime cost.

But users would have to switch to it, I don't think we can replace the
definition in bpf_helper_defs.h in a backwards compatible way.