Re: [PATCH bpf-next v12 12/25] bpf: Allow a subprog to end with a call that does not return

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

On 10/16/26 8:35 PM, bot+bpf-ci@kernel.org wrote:
>> bpf: Allow a subprog to end with a call that does not return
> TL;DR: Inserting exits after final calls may shift verifier log instruction
> indices for newly accepted programs, which could make libbpf's CO-RE
> relocation lookup miss or name the wrong field. Was this intended?
>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
> [ ... ]
>
>> @@ -3317,6 +3317,54 @@ static void find_subprog_properties(struct bpf_verifier_env *env)
>>   	}
>>   }
>>
>> +/*
>> + * Put an exit after a call that ends a subprog, so that nothing from the CFG
>> + * walk on has to know a subprog can end that way. check_subprogs() only lets
>> + * such a call be to a callee that never returns, so do_check() never reaches
>> + * the exit.
>> + */
>> +static int add_exits_after_final_calls(struct bpf_verifier_env *env)
>> +{
> [ ... ]
>
>> +		patch[0] = *insn;
>> +		new_prog = bpf_patch_insn_data(env, last, patch, 2);
>> +		if (!new_prog)
>> +			return -ENOMEM;
>> +		env->prog = new_prog;
>> +		/* The patch moved the call's aux data, orig_idx included, to the exit. */
>> +		env->insn_aux_data[last].orig_idx = env->insn_aux_data[last + 1].orig_idx;
> [ ... ]
>
>> +	}
>> +	return 0;
>> +}
> [ ... ]
>
>> @@ -23661,6 +23718,24 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr,
>>   	if (ret < 0)
>>   		goto skip_full_check;
>>
>> +	/*
>> +	 * After everything above has recorded insn offsets, so the patch
>> +	 * adjusts them, and before an offloaded program's per-insn state.
>> +	 */
>> +	ret = add_exits_after_final_calls(env);
>> +	if (ret < 0)
>> +		goto skip_full_check;
>> +
> Does inserting these exits before bpf_check_cfg() and do_check() shift the
> instruction indices that the verifier log reports?

I think this is indeed an issue, esp. for verifier log reporting.

For example, the original function:
    0: call foo
    1: call bpf_unwind
    2: call bpf_unwind_resume

After add_exits_after_final_calls, we will have
    idx  orig_idx
    0    0         call foo
    1    1         bpf_unwind
    2    1         exit
    3    2         call bpf_unwind_resume
    4    2         exit

When verifier trying to report error message, we cannot use 'idx'
any more, we have to use orig_idx. For example, we have
    verbose_linfo(env, env->insn_idx, "; ");
    ...
    verbose(env, "%d: ", env->insn_idx);

here env->insn_idx refers to 'idx' in the above,
but they do not correspond to the original idx.

The actual one may be replacing env->insn_idx to corresponding
orig_idx.

We could have a helper to map idx->orig_idx. This should work.
Do we have a better solution for this?

>
> Until now no pass changed the instruction count before verification, so the
> indices in the log matched the instructions user space loaded.  With this
> patch, every insn behind a subprog that ends with a call moves down by one
> for each exit added in front of it.  The CFG walk and do_check() print
> env->insn_idx, so every message about those insns now carries an index that
> is not in the loaded program.  insn_aux_data[].orig_idx exists for this kind
> of mismatch, but only a few messages use it.
>
> This has a concrete effect on libbpf.  fixup_log_failed_core_relo() in
> tools/lib/bpf/libbpf.c reads the index from the log and uses it to find the
> relocation:
>
>      if (sscanf(line1, "%d: (%*d) call unknown#195896080\n", &insn_idx) != 1)
>          return;
>      relo = find_relo_core(prog, insn_idx);
>
> find_relo_core() needs an exact relo->insn_idx match.  Take a program whose
> main subprog ends with "call _Unwind_Resume" (the prog() example in the
> commit message) and that has a poisoned CO-RE relocation at index N in a
> later subprog.  The kernel now logs that instruction as N+1, so libbpf
> either finds no relocation and leaves the raw "unknown#195896080" message,
> or picks up a different CO-RE relocation sitting at N+1 and names the wrong
> field.
>
> The cleanup-record messages are affected as well.  "resume at insn %d is not
> in a landing pad" and "insn %u ... is in a landing pad" now print shifted
> indices, while bpf_exc_check_info() reports the user's own offsets.
>
> Only programs that this patch newly accepts are affected, since
> add_exits_after_final_calls() patches nothing otherwise.  The commit message
> says the exits are put in "before the CFG walk" but does not mention the
> effect on the log indices.
>
> Was this intended?  Would it make sense to report orig_idx in the do_check()
> messages, or to handle a final call without inserting instructions until
> after verification?
>
>
> ---
> 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/38997507726