Re: [PATCH bpf-next v10 11/24] bpf: Prepare JITed programs for dispatching cleanup pads

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

On 10/9/26 7:13 AM, bot+bpf-ci@kernel.org wrote:
>> bpf: Prepare JITed programs for dispatching cleanup pads
> TL;DR: keep_subprog_exits() may mark an exit seen in a never-entered subprog,
> so dead-code removal can miscompute subprog starts and drop main's exit,
> letting JITed main fall off its image end.
>
>> diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
>> index 64baee1a37e6..1cc025c45ecd 100644
>> --- a/kernel/bpf/fixups.c
>> +++ b/kernel/bpf/fixups.c
> [ ... ]
>
>> @@ -715,6 +716,32 @@ static void keep_funcs_with_addr_taken(struct bpf_verifier_env *env)
>>   	}
>>   }
>>   
>> +static int keep_subprog_exits(struct bpf_verifier_env *env)
>> +{
>> +	u32 i, j;
>> +
>> +	for (i = 0; i < env->subprog_cnt; i++) {
>> +		bool found = false;
>> +		u32 start;
>> +
>> +		if (!env->subprog_info[i].might_unwind)
>> +			continue;
>> +		start = env->subprog_info[i].start;
>> +		for (j = env->subprog_info[i + 1].start; j-- > start; ) {
>> +			if (env->prog->insnsi[j].code != (BPF_JMP | BPF_EXIT))
>> +				continue;
>> +			env->insn_aux_data[j].seen = env->pass_cnt;
> Can this mark an exit as seen in a subprog the verifier never entered?
>
> might_unwind is set statically while walking the control flow, so it is also
> set on a static subprog that is only called from a path the verifier proves
> dead, for example one guarded by a .rodata constant or a constant register.
> None of that subprog's instructions are seen, but the loop above still marks
> its last exit as seen.
>
> Before this patch a live subprog was always entered at its first
> instruction, and adjust_subprog_starts_after_remove() relies on that.  Now
> the first instruction of a subprog can be dead while a later exit is kept.
> bpf_opt_remove_dead_code() removes each contiguous run of unseen
> instructions even when the run crosses a subprog boundary, so one run can
> cover the end of the previous code and the head of this subprog, and stop at
> the kept exit.
>
> Here is a trace (privileged loader, with bpf_unwind registered and the x86
> cleanup pad support from later patches in the series):
>
>    main:  0: r1 = 0
>           1: if r1 == 0 goto +2   ; always taken
>           2: call P               ; dead
>           3: call A               ; dead
>           4: r0 = 0
>           5: exit
>    P:     6: r0 = 1               ; static, never entered, no unwind
>           7: exit
>    A:     8: call bpf_unwind      ; static, never entered, might_unwind
>           9: r0 = 0
>          10: exit                 ; kept by keep_subprog_exits()
>
> The first dead run, [2,3], is removed correctly, leaving P.start = 4,
> A.start = 6 and the fake subprog at 9.
>
> The next dead run is [4..7]: all of P plus A's call and 'r0 = 0'.  It stops
> at A's kept exit, so off = 4 and cnt = 4.
>
> In adjust_subprog_starts_after_remove(), i is P.  The j loop reaches the fake
> subprog, whose start (9) is not off + cnt (8), so:
>
> 	if (env->subprog_info[j].start != off + cnt)
> 		j--;
>
> gives j = A.  Since j > i, P's entry is dropped and A moves into its slot,
> and then the start adjustment gives A.start = 6 - 4 = 2.  The correct value
> is 4.
>
> The program is now:
>
>    0: r1 = 0
>    1: ja +0
>    2: r0 = 0
>    3: exit
>    4: exit
>
> with subprog starts main = 0 and A = 2, so main has lost its own
> 'r0 = 0; exit'.  After bpf_opt_remove_nops() main is just [r1 = 0].
> jit_subprogs() then JITs func[0] with no exit, so it has no epilogue and no
> ret, and the first run of the program falls off the end of main's image.  If
> main had kept a jump into the reassigned tail, do_jit() would also read
> addrs[] past func[0]->len.
>
> The same miscalculation happens when P is a live subprog with only a dead
> tail (the i == j case, where start != off).
>
> Would it make sense to skip subprogs that were never entered, since they can
> never be on the stack during an unwind, for example:
>
> 	if (!env->insn_aux_data[start].seen)
> 		continue;

Indeed, this is a bug. Will fix.

>
> Alternatively, could adjust_subprog_starts_after_remove() set the start of a
> partially removed subprog to off?
>
> None of the later commits in this series appear to change keep_subprog_exits()
> or adjust_subprog_starts_after_remove().
>
>> +			found = true;
>> +			break;
>> +		}
>> +		if (!found) {
>> +			verbose(env, "subprog %u can be unwound through but has no exit\n", i);
>> +			return -EINVAL;
>> +		}
>> +	}
>> +	return 0;
>> +}
>> +
>
> ---
> AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
> See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
>
> CI run summary: https://github.com/kernel-patches/bpf/actions/runs/37884246994