Re: [PATCH bpf v3 1/2] bpf: Account for early exit of bpf_tail_call() and LD_ABS

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

On Mon Jan 6, 2025 at 9:31 PM CET, Eduard Zingerman wrote:
> On Mon, 2025-01-06 at 18:15 +0100, Arthur Fabre wrote:
[...]

> This patch is correct as far as I can tell.
>
> Acked-by: Eduard Zingerman <eddyz87@gmail.com>

Thanks for the review!

> [...]
>
> > @@ -18770,6 +18780,21 @@ static int do_check(struct bpf_verifier_env *env)
> >  					return err;
> >  
> >  				mark_reg_scratched(env, BPF_REG_0);
> > +
> > +				if (insn->src_reg == 0 && insn->imm == BPF_FUNC_tail_call) {
> > +					/* Explore both cases: tail_call fails and we fallthrough,
> > +					 * or it succeeds and we exit the current function.
> > +					 */
> > +					if (!push_stack(env, env->insn_idx + 1, env->insn_idx, false))
> > +						return -ENOMEM;
> > +					/* bpf_tail_call() doesn't set r0 on failure / in the fallthrough case.
> > +					 * But it does on success, so we have to mark it after queueing the
> > +					 * fallthrough case, but before prepare_func_exit().
> > +					 */
> > +					__mark_reg_unknown(env, &state->frame[state->curframe]->regs[BPF_REG_0]);
> > +					exit = BPF_EXIT_TAIL_CALL;
> > +					goto process_bpf_exit_full;
> > +				}
>
> Nit: it's a bit unfortunate, that this logic is inside do_check,
>      instead of check_helper_call() and check_ld_abs().
>      But it makes BPF_EXIT_* propagation simpler.

Agreed, it's unfortunate to add more to do_check().

I tried passing exit / BPF_EXIT_* by pointer to check_helper_call() and
check_ld_abs(), but that still means we need a conditional in do_check()
to see if it's set:

if (exit != NULL)
	oto process_bpf_exit_full;

Happy to switch to that if you think it's cleaner.

[...]