Re: [PATCH bpf-next v4 04/20] bpf: Prepare for an exception cleanup table before the CFG walk

[Eduard Zingerman <eddyz87@gmail.com>]

On Mon, 2026-09-21 at 14:00 -0700, Yonghong Song wrote:
> Some plumbing work is done before bpf_check_cfg(). More specifically,
> insn_aux_data records cleanup_throw_site for every bpf_throw() and
> cleanup_resume_site for every bpf_unwind_resume() -- the two calls a JIT
> lowers its own way rather than as calls -- and cleanup_pad, the landing pad
> a frame resumes at, for every call within the [begin_off, end_off) range of
> a cleanup record. Subsequent commits consume all three.
> 
> Marking the two calls here, rather than recognising them in the JIT, is
> what makes the recognition exact: by the time a JIT runs,
> bpf_fixup_kfunc_call() has rewritten every other kfunc's imm into an offset
> from __bpf_call_base, and a BTF id compared against one of those offsets
> could match an unrelated call.
> 
> bpf_prepare_cleanup_exceptions() runs before bpf_check_cfg(), because what
> it produces is what the CFG walk consumes. It refuses a table on an
> offloaded program, on a program whose JIT cannot dispatch landing pads or
> which the JIT was not asked to compile, and on a program that also installs
> an exception callback -- two different answers to what runs on the way out.
> bpf_jit_supports_cleanup_pads() is weak here and says no; the arch patches
> provide the real ones.
> 
> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
> ---

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

Just a few nits.

...

> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index 325a80ffcbe2..fdee9da6b45d 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -686,6 +686,8 @@ struct bpf_insn_aux_data {
>  	bool needs_zext; /* alu op needs to clear upper bits */
>  	bool non_sleepable; /* helper/kfunc may be called from non-sleepable context */
>  	bool is_iter_next; /* bpf_iter_<type>_next() kfunc call */
> +	bool cleanup_throw_site; /* call to bpf_throw() */
> +	bool cleanup_resume_site; /* call to bpf_unwind_resume() */

Nit: There are 12 bools here now, followed by a 25 bits hole,
     let's move `orig_idx` before or after flags and convert
     all the flags to bit fields. I think it can reduce the structure
     size from 144 to 136 bytes.
     Also, why not simply `throw_call` and `resume_call`?

>  	bool call_with_percpu_alloc_ptr; /* {this,per}_cpu_ptr() with prog percpu alloc */
>  	u8 alu_state; /* used in combination with alu_limit */
>  	/* true if STX or LDX instruction is a part of a spill/fill

...

> diff --git a/kernel/bpf/exception.c b/kernel/bpf/exception.c
> index 4b3ac93e98c1..67af78baa558 100644
> --- a/kernel/bpf/exception.c
> +++ b/kernel/bpf/exception.c
> @@ -18,6 +18,66 @@ static bool insn_is_unwind_resume(const struct bpf_insn *insn)
>  	       insn->imm == bpf_unwind_resume_id[0];
>  }
>  
> +static void cleanup_mark_kfunc_sites(struct bpf_verifier_env *env)

Nit: the 'cleanup_' prefix in function names triggers me a bit,
     as it is usually used when there are some cleanup actions are
     taken by the function. Maybe drop or reword it a bit?

> +{
> +	u32 i;
> +
> +	for (i = 0; i < env->prog->len; i++) {
> +		struct bpf_insn *insn = &env->prog->insnsi[i];
> +
> +		if (bpf_is_throw_kfunc(insn))
> +			env->insn_aux_data[i].cleanup_throw_site = true;
> +		else if (insn_is_unwind_resume(insn))
> +			env->insn_aux_data[i].cleanup_resume_site = true;
> +	}
> +}

...