Re: [PATCH bpf-next] bpf, verifier: Correct tail_call_reachable for bpf prog

[Leon Hwang <hffilwlqm@gmail.com>]

On 10/6/24 13:26, Yonghong Song wrote:
> 
> On 6/9/24 12:31 AM, Leon Hwang wrote:
>> It's confusing to inspect 'prog->aux->tail_call_reachable' with drgn[0],
>> when bpf prog has tail call but 'tail_call_reachable' is false.
>>
>> This patch corrects 'tail_call_reachable' when bpf prog has tail call.
>>
>> [0] https://github.com/osandov/drgn
>>
>> Signed-off-by: Leon Hwang <hffilwlqm@gmail.com>
>> ---
>>   kernel/bpf/verifier.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 81a3d2ced78d5..d7045676246a7 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -2982,8 +2982,10 @@ static int check_subprogs(struct
>> bpf_verifier_env *env)
>>             if (code == (BPF_JMP | BPF_CALL) &&
>>               insn[i].src_reg == 0 &&
>> -            insn[i].imm == BPF_FUNC_tail_call)
>> +            insn[i].imm == BPF_FUNC_tail_call) {
>>               subprog[cur_subprog].has_tail_call = true;
>> +            subprog[cur_subprog].tail_call_reachable = true;
> 
> This tail_call_reachable is handled in jit. For example, in
> arch/x86/net/bpf_jit_comp.c:
> 
> static void detect_reg_usage(struct bpf_insn *insn, int insn_cnt,
>                              bool *regs_used, bool *tail_call_seen)
> {
>         int i;
> 
>         for (i = 1; i <= insn_cnt; i++, insn++) {
>                 if (insn->code == (BPF_JMP | BPF_TAIL_CALL))
>                         *tail_call_seen = true;
>                 if (insn->dst_reg == BPF_REG_6 || insn->src_reg ==
> BPF_REG_6)
>                         regs_used[0] = true;
>                 if (insn->dst_reg == BPF_REG_7 || insn->src_reg ==
> BPF_REG_7)
>                         regs_used[1] = true;
>                 if (insn->dst_reg == BPF_REG_8 || insn->src_reg ==
> BPF_REG_8)
>                         regs_used[2] = true;
>                 if (insn->dst_reg == BPF_REG_9 || insn->src_reg ==
> BPF_REG_9)
>                         regs_used[3] = true;
>         }
> }
> 
> and
> 
>         detect_reg_usage(insn, insn_cnt, callee_regs_used,
>                          &tail_call_seen);
>                 /* tail call's presence in current prog implies it is
> reachable */
>         tail_call_reachable |= tail_call_seen;
> 
> I didn't check other architectures. If other arch is similar to x86 w.r.t.
> tail_call_reachable marking, your change looks good. But you should also
> make changes in jit to remove those redundent checking.
> 

By searching tail_call_reachable in arch directory, excluding x86, other
architectures do not check 'prog->aux->tail_call_reachable'.

By checking jit of arm64/loongarch/riscv/s390, they have their own way
to handle tail call, unlike x86's way to detect tail_call_reachable.

I'll send PATCH v2 to remove the redundant detecting in x86 jit.

Thanks,
Leon