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

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

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.

> +		}
>   		if (BPF_CLASS(code) == BPF_LD &&
>   		    (BPF_MODE(code) == BPF_ABS || BPF_MODE(code) == BPF_IND))
>   			subprog[cur_subprog].has_ld_abs = true;
>
> base-commit: 2c6987105026a4395935a3db665c54eb1bafe782