Re: [PATCH bpf-next v2 03/20] bpf: Add lookups for exception cleanup resumes and landing pads

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

On 9/18/26 9:55 PM, Alexei Starovoitov wrote:
> On Fri Sep 18, 2026 at 5:44 AM UTC, bot+bpf-ci wrote:
>>> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
>>> index c08505b9ba821..f9bccd3e0f4db 100644
>>> --- a/include/linux/bpf_verifier.h
>>> +++ b/include/linux/bpf_verifier.h
>>> @@ -681,6 +681,11 @@ 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 */
>>> +	/*
>>> +	 * 1 + the instruction index of the exception cleanup landing pad this
>>> +	 * call site unwinds to, or 0 for none.
>>> +	 */
>>> +	u32 cleanup_pad;
>>>   	bool call_with_percpu_alloc_ptr; /* {this,per}_cpu_ptr() with prog percpu alloc */
>>>   	u8 alu_state; /* used in combination with alu_limit */
>> [ ... ]
>>
>>> diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
>>> index 2add8001c3ec3..82b00fac6bd6a 100644
>>> --- a/kernel/bpf/fixups.c
>>> +++ b/kernel/bpf/fixups.c
>>> @@ -261,6 +261,11 @@ static void adjust_insn_aux_data(struct bpf_verifier_env *env,
>>>   		}
>>>   	}
>>>
>>> +	if (env->cleanup_info_cnt)
>>> +		for (i = 0; i < prog_len; i++)
>>> +			if (data[i].cleanup_pad > off + 1)
>>> +				data[i].cleanup_pad += cnt - 1;
>>> +
>> [ ... ]
>>
>>> @@ -549,6 +554,7 @@ static int verifier_remove_insns(struct bpf_verifier_env *env, u32 off, u32 cnt)
>>>   	struct bpf_insn_aux_data *aux_data = env->insn_aux_data;
>>>   	unsigned int orig_prog_len = env->prog->len;
>>>   	int err;
>>> +	u32 i;
>>>
>>>   	if (bpf_prog_is_offloaded(env->prog->aux))
>>>   		bpf_prog_offload_remove_insns(env, off, cnt);
>>> @@ -573,6 +579,17 @@ static int verifier_remove_insns(struct bpf_verifier_env *env, u32 off, u32 cnt)
>>>   		sizeof(*aux_data) * (orig_prog_len - off - cnt));
>>>   	env->insn_aux_data_len -= cnt;
>>>
>>> +	if (env->cleanup_info_cnt) {
>>> +		for (i = 0; i < env->insn_aux_data_len; i++) {
>>> +			u32 pad = aux_data[i].cleanup_pad;
>>> +
>>> +			if (pad > off + cnt)
>>> +				aux_data[i].cleanup_pad = pad - cnt;
>>> +			else if (pad > off)
>>> +				aux_data[i].cleanup_pad = 0;
>>> +		}
>>> +	}
>>> +
>>>   	return 0;
>>>   }
>> Can this logic handle NOP removal correctly? When bpf_opt_remove_nops()
>> calls verifier_remove_insns(env, i, 1) to remove a single fall-through
>> goto +0, if a cleanup record names that instruction as its landing pad,
>> then pad == off + 1, so pad > off is true and cleanup_pad becomes 0.
>>
>> But removing the NOP leaves the pad's next instruction at the same
>> index, so the correct landing pad index stays unchanged rather than
>> being zeroed. For example:
>>
>>      off:     some_call with cleanup_pad = off+1
>>      off+1:   goto +0  (NOP landing pad)
>>      off+2:   actual cleanup code
>>
>> After removing the NOP at off+1:
>>
>>      off:     some_call (cleanup_pad should still be off+1)
>>      off+1:   actual cleanup code (was at off+2)
>>
>> The `else if (pad > off)` arm erases the record instead of preserving
>> it. Nothing rejects NOP landing pads during validation - a goto +0 has
>> code 0x05, not 0x00, so check_cleanup_info() accepts it.
>>
>> The ordering makes the loss escape validation: bpf_check_cleanup_exceptions()
>> runs before bpf_opt_remove_nops(), so the verifier proves the cleanup
>> path releases acquired references, but then the NOP removal zeroes
>> cleanup_pad, and later cleanup_info_for_subprog() sees `if (!pad)
>> continue;` and omits the record from the runtime table. The program is
>> accepted but bpf_throw() finds no pad for that call site and skips the
>> cleanup, leaking whatever the pad was supposed to release.
>>
>> Does the `else if (pad > off)` arm need to distinguish removals that
>> consume the landing pad instruction itself versus removals that happen
>> before it?
> bot is correct here.
> pls fix

Agree with bot about for two different cases. Will fix.

>
> pw-bot: cr