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

["Alexei Starovoitov" <alexei.starovoitov@gmail.com>]

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

pw-bot: cr