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

[Yonghong Song <yonghong.song@linux.dev>]

On 9/22/26 11:27 AM, Eduard Zingerman wrote:
> 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.

I will make these bool as bitfield to reduce the struct size.

>       Also, why not simply `throw_call` and `resume_call`?

Will do.

>
>>   	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?

Yes, I will try to avoid cleanup_ prefix then. For static functions,
I will not have cleanup_ prefix or may use exc_ prefix.
For global function, I will bpf_cleanup_ as prefix.

>
>> +{
>> +	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;
>> +	}
>> +}
> ...