* [PATCH bpf-next] bpf, verifier: Correct tail_call_reachable for bpf prog @ 2024-06-09 7:31 Leon Hwang 2024-06-10 5:26 ` Yonghong Song 0 siblings, 1 reply; 3+ messages in thread From: Leon Hwang @ 2024-06-09 7:31 UTC (permalink / raw) To: bpf; +Cc: ast, daniel, hffilwlqm, kernel-patches-bot 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; + } 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 -- 2.44.0 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH bpf-next] bpf, verifier: Correct tail_call_reachable for bpf prog 2024-06-09 7:31 [PATCH bpf-next] bpf, verifier: Correct tail_call_reachable for bpf prog Leon Hwang @ 2024-06-10 5:26 ` Yonghong Song 2024-06-10 7:12 ` Leon Hwang 0 siblings, 1 reply; 3+ messages in thread From: Yonghong Song @ 2024-06-10 5:26 UTC (permalink / raw) To: Leon Hwang, bpf; +Cc: ast, daniel, kernel-patches-bot 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH bpf-next] bpf, verifier: Correct tail_call_reachable for bpf prog 2024-06-10 5:26 ` Yonghong Song @ 2024-06-10 7:12 ` Leon Hwang 0 siblings, 0 replies; 3+ messages in thread From: Leon Hwang @ 2024-06-10 7:12 UTC (permalink / raw) To: Yonghong Song, bpf; +Cc: ast, daniel, kernel-patches-bot 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-06-10 7:12 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-06-09 7:31 [PATCH bpf-next] bpf, verifier: Correct tail_call_reachable for bpf prog Leon Hwang 2024-06-10 5:26 ` Yonghong Song 2024-06-10 7:12 ` Leon Hwang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox