* [PATCH bpf-next 1/2] bpf: Reduce stack frame size by using env->insn_buf for bpf insns
@ 2025-07-02 5:33 Yonghong Song
2025-07-02 5:33 ` [PATCH bpf-next 2/2] bpf: Avoid putting struct bpf_scc_callchain variables on the stack Yonghong Song
` (3 more replies)
0 siblings, 4 replies; 7+ messages in thread
From: Yonghong Song @ 2025-07-02 5:33 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, kernel-team,
Martin KaFai Lau, Arnd Bergmann
Arnd Bergmann reported an issue ([1]) where clang compiler (less than
llvm18) may trigger an error where the stack frame size exceeds the limit.
I can reproduce the error like below:
kernel/bpf/verifier.c:24491:5: error: stack frame size (2552) exceeds limit (1280) in 'bpf_check'
[-Werror,-Wframe-larger-than]
kernel/bpf/verifier.c:19921:12: error: stack frame size (1368) exceeds limit (1280) in 'do_check'
[-Werror,-Wframe-larger-than]
Use env->insn_buf for bpf insns instead of putting these insns on the
stack. This can resolve the above 'bpf_check' error. The 'do_check' error
will be resolved in the next patch.
[1] https://lore.kernel.org/bpf/20250620113846.3950478-1-arnd@kernel.org/
Reported-by: Arnd Bergmann <arnd@kernel.org>
Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
---
kernel/bpf/verifier.c | 194 ++++++++++++++++++++----------------------
1 file changed, 91 insertions(+), 103 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 90e688f81a48..29faef51065d 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -20939,26 +20939,27 @@ static bool insn_is_cond_jump(u8 code)
static void opt_hard_wire_dead_code_branches(struct bpf_verifier_env *env)
{
struct bpf_insn_aux_data *aux_data = env->insn_aux_data;
- struct bpf_insn ja = BPF_JMP_IMM(BPF_JA, 0, 0, 0);
+ struct bpf_insn *ja = env->insn_buf;
struct bpf_insn *insn = env->prog->insnsi;
const int insn_cnt = env->prog->len;
int i;
+ *ja = BPF_JMP_IMM(BPF_JA, 0, 0, 0);
for (i = 0; i < insn_cnt; i++, insn++) {
if (!insn_is_cond_jump(insn->code))
continue;
if (!aux_data[i + 1].seen)
- ja.off = insn->off;
+ ja->off = insn->off;
else if (!aux_data[i + 1 + insn->off].seen)
- ja.off = 0;
+ ja->off = 0;
else
continue;
if (bpf_prog_is_offloaded(env->prog->aux))
- bpf_prog_offload_replace_insn(env, i, &ja);
+ bpf_prog_offload_replace_insn(env, i, ja);
- memcpy(insn, &ja, sizeof(ja));
+ memcpy(insn, ja, sizeof(*ja));
}
}
@@ -21017,7 +21018,9 @@ static int opt_remove_nops(struct bpf_verifier_env *env)
static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env,
const union bpf_attr *attr)
{
- struct bpf_insn *patch, zext_patch[2], rnd_hi32_patch[4];
+ struct bpf_insn *patch;
+ struct bpf_insn *zext_patch = env->insn_buf;
+ struct bpf_insn *rnd_hi32_patch = &env->insn_buf[2];
struct bpf_insn_aux_data *aux = env->insn_aux_data;
int i, patch_len, delta = 0, len = env->prog->len;
struct bpf_insn *insns = env->prog->insnsi;
@@ -21195,13 +21198,12 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
if (env->insn_aux_data[i + delta].nospec) {
WARN_ON_ONCE(env->insn_aux_data[i + delta].alu_state);
- struct bpf_insn patch[] = {
- BPF_ST_NOSPEC(),
- *insn,
- };
+ struct bpf_insn *patch = &insn_buf[0];
- cnt = ARRAY_SIZE(patch);
- new_prog = bpf_patch_insn_data(env, i + delta, patch, cnt);
+ *patch++ = BPF_ST_NOSPEC();
+ *patch++ = *insn;
+ cnt = patch - insn_buf;
+ new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, cnt);
if (!new_prog)
return -ENOMEM;
@@ -21269,13 +21271,12 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
/* nospec_result is only used to mitigate Spectre v4 and
* to limit verification-time for Spectre v1.
*/
- struct bpf_insn patch[] = {
- *insn,
- BPF_ST_NOSPEC(),
- };
+ struct bpf_insn *patch = &insn_buf[0];
- cnt = ARRAY_SIZE(patch);
- new_prog = bpf_patch_insn_data(env, i + delta, patch, cnt);
+ *patch++ = *insn;
+ *patch++ = BPF_ST_NOSPEC();
+ cnt = patch - insn_buf;
+ new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, cnt);
if (!new_prog)
return -ENOMEM;
@@ -21945,13 +21946,12 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
u16 stack_depth_extra = 0;
if (env->seen_exception && !env->exception_callback_subprog) {
- struct bpf_insn patch[] = {
- env->prog->insnsi[insn_cnt - 1],
- BPF_MOV64_REG(BPF_REG_0, BPF_REG_1),
- BPF_EXIT_INSN(),
- };
+ struct bpf_insn *patch = &insn_buf[0];
- ret = add_hidden_subprog(env, patch, ARRAY_SIZE(patch));
+ *patch++ = env->prog->insnsi[insn_cnt - 1];
+ *patch++ = BPF_MOV64_REG(BPF_REG_0, BPF_REG_1);
+ *patch++ = BPF_EXIT_INSN();
+ ret = add_hidden_subprog(env, insn_buf, patch - insn_buf);
if (ret < 0)
return ret;
prog = env->prog;
@@ -21987,20 +21987,18 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
insn->off == 1 && insn->imm == -1) {
bool is64 = BPF_CLASS(insn->code) == BPF_ALU64;
bool isdiv = BPF_OP(insn->code) == BPF_DIV;
- struct bpf_insn *patchlet;
- struct bpf_insn chk_and_sdiv[] = {
- BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
- BPF_NEG | BPF_K, insn->dst_reg,
- 0, 0, 0),
- };
- struct bpf_insn chk_and_smod[] = {
- BPF_MOV32_IMM(insn->dst_reg, 0),
- };
+ struct bpf_insn *patch = &insn_buf[0];
+
+ if (isdiv)
+ *patch++ = BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
+ BPF_NEG | BPF_K, insn->dst_reg,
+ 0, 0, 0);
+ else
+ *patch++ = BPF_MOV32_IMM(insn->dst_reg, 0);
- patchlet = isdiv ? chk_and_sdiv : chk_and_smod;
- cnt = isdiv ? ARRAY_SIZE(chk_and_sdiv) : ARRAY_SIZE(chk_and_smod);
+ cnt = patch - insn_buf;
- new_prog = bpf_patch_insn_data(env, i + delta, patchlet, cnt);
+ new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, cnt);
if (!new_prog)
return -ENOMEM;
@@ -22019,83 +22017,73 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
bool isdiv = BPF_OP(insn->code) == BPF_DIV;
bool is_sdiv = isdiv && insn->off == 1;
bool is_smod = !isdiv && insn->off == 1;
- struct bpf_insn *patchlet;
- struct bpf_insn chk_and_div[] = {
- /* [R,W]x div 0 -> 0 */
- BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
- BPF_JNE | BPF_K, insn->src_reg,
- 0, 2, 0),
- BPF_ALU32_REG(BPF_XOR, insn->dst_reg, insn->dst_reg),
- BPF_JMP_IMM(BPF_JA, 0, 0, 1),
- *insn,
- };
- struct bpf_insn chk_and_mod[] = {
- /* [R,W]x mod 0 -> [R,W]x */
- BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
- BPF_JEQ | BPF_K, insn->src_reg,
- 0, 1 + (is64 ? 0 : 1), 0),
- *insn,
- BPF_JMP_IMM(BPF_JA, 0, 0, 1),
- BPF_MOV32_REG(insn->dst_reg, insn->dst_reg),
- };
- struct bpf_insn chk_and_sdiv[] = {
+ struct bpf_insn *patch = &insn_buf[0];
+
+ if (is_sdiv) {
/* [R,W]x sdiv 0 -> 0
* LLONG_MIN sdiv -1 -> LLONG_MIN
* INT_MIN sdiv -1 -> INT_MIN
*/
- BPF_MOV64_REG(BPF_REG_AX, insn->src_reg),
- BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
- BPF_ADD | BPF_K, BPF_REG_AX,
- 0, 0, 1),
- BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
- BPF_JGT | BPF_K, BPF_REG_AX,
- 0, 4, 1),
- BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
- BPF_JEQ | BPF_K, BPF_REG_AX,
- 0, 1, 0),
- BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
- BPF_MOV | BPF_K, insn->dst_reg,
- 0, 0, 0),
+ *patch++ = BPF_MOV64_REG(BPF_REG_AX, insn->src_reg);
+ *patch++ = BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
+ BPF_ADD | BPF_K, BPF_REG_AX,
+ 0, 0, 1);
+ *patch++ = BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
+ BPF_JGT | BPF_K, BPF_REG_AX,
+ 0, 4, 1);
+ *patch++ = BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
+ BPF_JEQ | BPF_K, BPF_REG_AX,
+ 0, 1, 0);
+ *patch++ = BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
+ BPF_MOV | BPF_K, insn->dst_reg,
+ 0, 0, 0);
/* BPF_NEG(LLONG_MIN) == -LLONG_MIN == LLONG_MIN */
- BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
- BPF_NEG | BPF_K, insn->dst_reg,
- 0, 0, 0),
- BPF_JMP_IMM(BPF_JA, 0, 0, 1),
- *insn,
- };
- struct bpf_insn chk_and_smod[] = {
+ *patch++ = BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
+ BPF_NEG | BPF_K, insn->dst_reg,
+ 0, 0, 0);
+ *patch++ = BPF_JMP_IMM(BPF_JA, 0, 0, 1);
+ *patch++ = *insn;
+ cnt = patch - insn_buf;
+ } else if (is_smod) {
/* [R,W]x mod 0 -> [R,W]x */
/* [R,W]x mod -1 -> 0 */
- BPF_MOV64_REG(BPF_REG_AX, insn->src_reg),
- BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
- BPF_ADD | BPF_K, BPF_REG_AX,
- 0, 0, 1),
- BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
- BPF_JGT | BPF_K, BPF_REG_AX,
- 0, 3, 1),
- BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
- BPF_JEQ | BPF_K, BPF_REG_AX,
- 0, 3 + (is64 ? 0 : 1), 1),
- BPF_MOV32_IMM(insn->dst_reg, 0),
- BPF_JMP_IMM(BPF_JA, 0, 0, 1),
- *insn,
- BPF_JMP_IMM(BPF_JA, 0, 0, 1),
- BPF_MOV32_REG(insn->dst_reg, insn->dst_reg),
- };
-
- if (is_sdiv) {
- patchlet = chk_and_sdiv;
- cnt = ARRAY_SIZE(chk_and_sdiv);
- } else if (is_smod) {
- patchlet = chk_and_smod;
- cnt = ARRAY_SIZE(chk_and_smod) - (is64 ? 2 : 0);
+ *patch++ = BPF_MOV64_REG(BPF_REG_AX, insn->src_reg);
+ *patch++ = BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
+ BPF_ADD | BPF_K, BPF_REG_AX,
+ 0, 0, 1);
+ *patch++ = BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
+ BPF_JGT | BPF_K, BPF_REG_AX,
+ 0, 3, 1);
+ *patch++ = BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
+ BPF_JEQ | BPF_K, BPF_REG_AX,
+ 0, 3 + (is64 ? 0 : 1), 1);
+ *patch++ = BPF_MOV32_IMM(insn->dst_reg, 0);
+ *patch++ = BPF_JMP_IMM(BPF_JA, 0, 0, 1);
+ *patch++ = *insn;
+ *patch++ = BPF_JMP_IMM(BPF_JA, 0, 0, 1);
+ *patch++ = BPF_MOV32_REG(insn->dst_reg, insn->dst_reg);
+ cnt = (patch - insn_buf) - (is64 ? 2 : 0);
+ } else if (isdiv) {
+ /* [R,W]x div 0 -> 0 */
+ *patch++ = BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
+ BPF_JNE | BPF_K, insn->src_reg,
+ 0, 2, 0);
+ *patch++ = BPF_ALU32_REG(BPF_XOR, insn->dst_reg, insn->dst_reg);
+ *patch++ = BPF_JMP_IMM(BPF_JA, 0, 0, 1);
+ *patch++ = *insn;
+ cnt = patch - insn_buf;
} else {
- patchlet = isdiv ? chk_and_div : chk_and_mod;
- cnt = isdiv ? ARRAY_SIZE(chk_and_div) :
- ARRAY_SIZE(chk_and_mod) - (is64 ? 2 : 0);
+ /* [R,W]x mod 0 -> [R,W]x */
+ *patch++ = BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
+ BPF_JEQ | BPF_K, insn->src_reg,
+ 0, 1 + (is64 ? 0 : 1), 0);
+ *patch++ = *insn;
+ *patch++ = BPF_JMP_IMM(BPF_JA, 0, 0, 1);
+ *patch++ = BPF_MOV32_REG(insn->dst_reg, insn->dst_reg);
+ cnt = (patch - insn_buf) - (is64 ? 2 : 0);
}
- new_prog = bpf_patch_insn_data(env, i + delta, patchlet, cnt);
+ new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, cnt);
if (!new_prog)
return -ENOMEM;
--
2.47.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH bpf-next 2/2] bpf: Avoid putting struct bpf_scc_callchain variables on the stack
2025-07-02 5:33 [PATCH bpf-next 1/2] bpf: Reduce stack frame size by using env->insn_buf for bpf insns Yonghong Song
@ 2025-07-02 5:33 ` Yonghong Song
2025-07-02 8:49 ` Jiri Olsa
2025-07-02 8:54 ` [PATCH bpf-next 1/2] bpf: Reduce stack frame size by using env->insn_buf for bpf insns Jiri Olsa
` (2 subsequent siblings)
3 siblings, 1 reply; 7+ messages in thread
From: Yonghong Song @ 2025-07-02 5:33 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, kernel-team,
Martin KaFai Lau, Arnd Bergmann
Add a 'struct bpf_scc_callchain callchain' field in bpf_verifier_env.
This way, the previous bpf_scc_callchain local variables can be
replaced by taking address of env->callchain. This can reduce stack
usage and fix the following error:
kernel/bpf/verifier.c:19921:12: error: stack frame size (1368) exceeds limit (1280) in 'do_check'
[-Werror,-Wframe-larger-than]
Reported-by: Arnd Bergmann <arnd@kernel.org>
Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
---
include/linux/bpf_verifier.h | 1 +
kernel/bpf/verifier.c | 36 ++++++++++++++++++------------------
2 files changed, 19 insertions(+), 18 deletions(-)
diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index 7e459e839f8b..e2c175d608bb 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -841,6 +841,7 @@ struct bpf_verifier_env {
char tmp_str_buf[TMP_STR_BUF_LEN];
struct bpf_insn insn_buf[INSN_BUF_SIZE];
struct bpf_insn epilogue_buf[INSN_BUF_SIZE];
+ struct bpf_scc_callchain callchain;
/* array of pointers to bpf_scc_info indexed by SCC id */
struct bpf_scc_info **scc_info;
u32 scc_cnt;
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 29faef51065d..b334e6434eb4 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -1913,19 +1913,19 @@ static char *format_callchain(struct bpf_verifier_env *env, struct bpf_scc_callc
*/
static int maybe_enter_scc(struct bpf_verifier_env *env, struct bpf_verifier_state *st)
{
- struct bpf_scc_callchain callchain;
+ struct bpf_scc_callchain *callchain = &env->callchain;
struct bpf_scc_visit *visit;
- if (!compute_scc_callchain(env, st, &callchain))
+ if (!compute_scc_callchain(env, st, callchain))
return 0;
- visit = scc_visit_lookup(env, &callchain);
- visit = visit ?: scc_visit_alloc(env, &callchain);
+ visit = scc_visit_lookup(env, callchain);
+ visit = visit ?: scc_visit_alloc(env, callchain);
if (!visit)
return -ENOMEM;
if (!visit->entry_state) {
visit->entry_state = st;
if (env->log.level & BPF_LOG_LEVEL2)
- verbose(env, "SCC enter %s\n", format_callchain(env, &callchain));
+ verbose(env, "SCC enter %s\n", format_callchain(env, callchain));
}
return 0;
}
@@ -1938,21 +1938,21 @@ static int propagate_backedges(struct bpf_verifier_env *env, struct bpf_scc_visi
*/
static int maybe_exit_scc(struct bpf_verifier_env *env, struct bpf_verifier_state *st)
{
- struct bpf_scc_callchain callchain;
+ struct bpf_scc_callchain *callchain = &env->callchain;
struct bpf_scc_visit *visit;
- if (!compute_scc_callchain(env, st, &callchain))
+ if (!compute_scc_callchain(env, st, callchain))
return 0;
- visit = scc_visit_lookup(env, &callchain);
+ visit = scc_visit_lookup(env, callchain);
if (!visit) {
verifier_bug(env, "scc exit: no visit info for call chain %s",
- format_callchain(env, &callchain));
+ format_callchain(env, callchain));
return -EFAULT;
}
if (visit->entry_state != st)
return 0;
if (env->log.level & BPF_LOG_LEVEL2)
- verbose(env, "SCC exit %s\n", format_callchain(env, &callchain));
+ verbose(env, "SCC exit %s\n", format_callchain(env, callchain));
visit->entry_state = NULL;
env->num_backedges -= visit->num_backedges;
visit->num_backedges = 0;
@@ -1967,22 +1967,22 @@ static int add_scc_backedge(struct bpf_verifier_env *env,
struct bpf_verifier_state *st,
struct bpf_scc_backedge *backedge)
{
- struct bpf_scc_callchain callchain;
+ struct bpf_scc_callchain *callchain = &env->callchain;
struct bpf_scc_visit *visit;
- if (!compute_scc_callchain(env, st, &callchain)) {
+ if (!compute_scc_callchain(env, st, callchain)) {
verifier_bug(env, "add backedge: no SCC in verification path, insn_idx %d",
st->insn_idx);
return -EFAULT;
}
- visit = scc_visit_lookup(env, &callchain);
+ visit = scc_visit_lookup(env, callchain);
if (!visit) {
verifier_bug(env, "add backedge: no visit info for call chain %s",
- format_callchain(env, &callchain));
+ format_callchain(env, callchain));
return -EFAULT;
}
if (env->log.level & BPF_LOG_LEVEL2)
- verbose(env, "SCC backedge %s\n", format_callchain(env, &callchain));
+ verbose(env, "SCC backedge %s\n", format_callchain(env, callchain));
backedge->next = visit->backedges;
visit->backedges = backedge;
visit->num_backedges++;
@@ -1998,12 +1998,12 @@ static int add_scc_backedge(struct bpf_verifier_env *env,
static bool incomplete_read_marks(struct bpf_verifier_env *env,
struct bpf_verifier_state *st)
{
- struct bpf_scc_callchain callchain;
+ struct bpf_scc_callchain *callchain = &env->callchain;
struct bpf_scc_visit *visit;
- if (!compute_scc_callchain(env, st, &callchain))
+ if (!compute_scc_callchain(env, st, callchain))
return false;
- visit = scc_visit_lookup(env, &callchain);
+ visit = scc_visit_lookup(env, callchain);
if (!visit)
return false;
return !!visit->backedges;
--
2.47.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next 2/2] bpf: Avoid putting struct bpf_scc_callchain variables on the stack
2025-07-02 5:33 ` [PATCH bpf-next 2/2] bpf: Avoid putting struct bpf_scc_callchain variables on the stack Yonghong Song
@ 2025-07-02 8:49 ` Jiri Olsa
0 siblings, 0 replies; 7+ messages in thread
From: Jiri Olsa @ 2025-07-02 8:49 UTC (permalink / raw)
To: Yonghong Song
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
kernel-team, Martin KaFai Lau, Arnd Bergmann
On Tue, Jul 01, 2025 at 10:33:37PM -0700, Yonghong Song wrote:
> Add a 'struct bpf_scc_callchain callchain' field in bpf_verifier_env.
> This way, the previous bpf_scc_callchain local variables can be
> replaced by taking address of env->callchain. This can reduce stack
> usage and fix the following error:
> kernel/bpf/verifier.c:19921:12: error: stack frame size (1368) exceeds limit (1280) in 'do_check'
> [-Werror,-Wframe-larger-than]
>
> Reported-by: Arnd Bergmann <arnd@kernel.org>
> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
Acked-by: Jiri Olsa <jolsa@kernel.org>
jirka
> ---
> include/linux/bpf_verifier.h | 1 +
> kernel/bpf/verifier.c | 36 ++++++++++++++++++------------------
> 2 files changed, 19 insertions(+), 18 deletions(-)
>
> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index 7e459e839f8b..e2c175d608bb 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -841,6 +841,7 @@ struct bpf_verifier_env {
> char tmp_str_buf[TMP_STR_BUF_LEN];
> struct bpf_insn insn_buf[INSN_BUF_SIZE];
> struct bpf_insn epilogue_buf[INSN_BUF_SIZE];
> + struct bpf_scc_callchain callchain;
> /* array of pointers to bpf_scc_info indexed by SCC id */
> struct bpf_scc_info **scc_info;
> u32 scc_cnt;
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 29faef51065d..b334e6434eb4 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -1913,19 +1913,19 @@ static char *format_callchain(struct bpf_verifier_env *env, struct bpf_scc_callc
> */
> static int maybe_enter_scc(struct bpf_verifier_env *env, struct bpf_verifier_state *st)
> {
> - struct bpf_scc_callchain callchain;
> + struct bpf_scc_callchain *callchain = &env->callchain;
> struct bpf_scc_visit *visit;
>
> - if (!compute_scc_callchain(env, st, &callchain))
> + if (!compute_scc_callchain(env, st, callchain))
> return 0;
> - visit = scc_visit_lookup(env, &callchain);
> - visit = visit ?: scc_visit_alloc(env, &callchain);
> + visit = scc_visit_lookup(env, callchain);
> + visit = visit ?: scc_visit_alloc(env, callchain);
> if (!visit)
> return -ENOMEM;
> if (!visit->entry_state) {
> visit->entry_state = st;
> if (env->log.level & BPF_LOG_LEVEL2)
> - verbose(env, "SCC enter %s\n", format_callchain(env, &callchain));
> + verbose(env, "SCC enter %s\n", format_callchain(env, callchain));
> }
> return 0;
> }
> @@ -1938,21 +1938,21 @@ static int propagate_backedges(struct bpf_verifier_env *env, struct bpf_scc_visi
> */
> static int maybe_exit_scc(struct bpf_verifier_env *env, struct bpf_verifier_state *st)
> {
> - struct bpf_scc_callchain callchain;
> + struct bpf_scc_callchain *callchain = &env->callchain;
> struct bpf_scc_visit *visit;
>
> - if (!compute_scc_callchain(env, st, &callchain))
> + if (!compute_scc_callchain(env, st, callchain))
> return 0;
> - visit = scc_visit_lookup(env, &callchain);
> + visit = scc_visit_lookup(env, callchain);
> if (!visit) {
> verifier_bug(env, "scc exit: no visit info for call chain %s",
> - format_callchain(env, &callchain));
> + format_callchain(env, callchain));
> return -EFAULT;
> }
> if (visit->entry_state != st)
> return 0;
> if (env->log.level & BPF_LOG_LEVEL2)
> - verbose(env, "SCC exit %s\n", format_callchain(env, &callchain));
> + verbose(env, "SCC exit %s\n", format_callchain(env, callchain));
> visit->entry_state = NULL;
> env->num_backedges -= visit->num_backedges;
> visit->num_backedges = 0;
> @@ -1967,22 +1967,22 @@ static int add_scc_backedge(struct bpf_verifier_env *env,
> struct bpf_verifier_state *st,
> struct bpf_scc_backedge *backedge)
> {
> - struct bpf_scc_callchain callchain;
> + struct bpf_scc_callchain *callchain = &env->callchain;
> struct bpf_scc_visit *visit;
>
> - if (!compute_scc_callchain(env, st, &callchain)) {
> + if (!compute_scc_callchain(env, st, callchain)) {
> verifier_bug(env, "add backedge: no SCC in verification path, insn_idx %d",
> st->insn_idx);
> return -EFAULT;
> }
> - visit = scc_visit_lookup(env, &callchain);
> + visit = scc_visit_lookup(env, callchain);
> if (!visit) {
> verifier_bug(env, "add backedge: no visit info for call chain %s",
> - format_callchain(env, &callchain));
> + format_callchain(env, callchain));
> return -EFAULT;
> }
> if (env->log.level & BPF_LOG_LEVEL2)
> - verbose(env, "SCC backedge %s\n", format_callchain(env, &callchain));
> + verbose(env, "SCC backedge %s\n", format_callchain(env, callchain));
> backedge->next = visit->backedges;
> visit->backedges = backedge;
> visit->num_backedges++;
> @@ -1998,12 +1998,12 @@ static int add_scc_backedge(struct bpf_verifier_env *env,
> static bool incomplete_read_marks(struct bpf_verifier_env *env,
> struct bpf_verifier_state *st)
> {
> - struct bpf_scc_callchain callchain;
> + struct bpf_scc_callchain *callchain = &env->callchain;
> struct bpf_scc_visit *visit;
>
> - if (!compute_scc_callchain(env, st, &callchain))
> + if (!compute_scc_callchain(env, st, callchain))
> return false;
> - visit = scc_visit_lookup(env, &callchain);
> + visit = scc_visit_lookup(env, callchain);
> if (!visit)
> return false;
> return !!visit->backedges;
> --
> 2.47.1
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: Reduce stack frame size by using env->insn_buf for bpf insns
2025-07-02 5:33 [PATCH bpf-next 1/2] bpf: Reduce stack frame size by using env->insn_buf for bpf insns Yonghong Song
2025-07-02 5:33 ` [PATCH bpf-next 2/2] bpf: Avoid putting struct bpf_scc_callchain variables on the stack Yonghong Song
@ 2025-07-02 8:54 ` Jiri Olsa
2025-07-02 9:28 ` Arnd Bergmann
2025-07-02 15:09 ` Alexei Starovoitov
3 siblings, 0 replies; 7+ messages in thread
From: Jiri Olsa @ 2025-07-02 8:54 UTC (permalink / raw)
To: Yonghong Song
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
kernel-team, Martin KaFai Lau, Arnd Bergmann
On Tue, Jul 01, 2025 at 10:33:32PM -0700, Yonghong Song wrote:
> Arnd Bergmann reported an issue ([1]) where clang compiler (less than
> llvm18) may trigger an error where the stack frame size exceeds the limit.
> I can reproduce the error like below:
> kernel/bpf/verifier.c:24491:5: error: stack frame size (2552) exceeds limit (1280) in 'bpf_check'
> [-Werror,-Wframe-larger-than]
> kernel/bpf/verifier.c:19921:12: error: stack frame size (1368) exceeds limit (1280) in 'do_check'
> [-Werror,-Wframe-larger-than]
>
> Use env->insn_buf for bpf insns instead of putting these insns on the
> stack. This can resolve the above 'bpf_check' error. The 'do_check' error
> will be resolved in the next patch.
>
> [1] https://lore.kernel.org/bpf/20250620113846.3950478-1-arnd@kernel.org/
>
> Reported-by: Arnd Bergmann <arnd@kernel.org>
> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
lgtm
Acked-by: Jiri Olsa <jolsa@kernel.org>
jirka
> ---
> kernel/bpf/verifier.c | 194 ++++++++++++++++++++----------------------
> 1 file changed, 91 insertions(+), 103 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 90e688f81a48..29faef51065d 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -20939,26 +20939,27 @@ static bool insn_is_cond_jump(u8 code)
> static void opt_hard_wire_dead_code_branches(struct bpf_verifier_env *env)
> {
> struct bpf_insn_aux_data *aux_data = env->insn_aux_data;
> - struct bpf_insn ja = BPF_JMP_IMM(BPF_JA, 0, 0, 0);
> + struct bpf_insn *ja = env->insn_buf;
> struct bpf_insn *insn = env->prog->insnsi;
> const int insn_cnt = env->prog->len;
> int i;
>
> + *ja = BPF_JMP_IMM(BPF_JA, 0, 0, 0);
> for (i = 0; i < insn_cnt; i++, insn++) {
> if (!insn_is_cond_jump(insn->code))
> continue;
>
> if (!aux_data[i + 1].seen)
> - ja.off = insn->off;
> + ja->off = insn->off;
> else if (!aux_data[i + 1 + insn->off].seen)
> - ja.off = 0;
> + ja->off = 0;
> else
> continue;
>
> if (bpf_prog_is_offloaded(env->prog->aux))
> - bpf_prog_offload_replace_insn(env, i, &ja);
> + bpf_prog_offload_replace_insn(env, i, ja);
>
> - memcpy(insn, &ja, sizeof(ja));
> + memcpy(insn, ja, sizeof(*ja));
> }
> }
>
> @@ -21017,7 +21018,9 @@ static int opt_remove_nops(struct bpf_verifier_env *env)
> static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env,
> const union bpf_attr *attr)
> {
> - struct bpf_insn *patch, zext_patch[2], rnd_hi32_patch[4];
> + struct bpf_insn *patch;
> + struct bpf_insn *zext_patch = env->insn_buf;
> + struct bpf_insn *rnd_hi32_patch = &env->insn_buf[2];
> struct bpf_insn_aux_data *aux = env->insn_aux_data;
> int i, patch_len, delta = 0, len = env->prog->len;
> struct bpf_insn *insns = env->prog->insnsi;
> @@ -21195,13 +21198,12 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
>
> if (env->insn_aux_data[i + delta].nospec) {
> WARN_ON_ONCE(env->insn_aux_data[i + delta].alu_state);
> - struct bpf_insn patch[] = {
> - BPF_ST_NOSPEC(),
> - *insn,
> - };
> + struct bpf_insn *patch = &insn_buf[0];
>
> - cnt = ARRAY_SIZE(patch);
> - new_prog = bpf_patch_insn_data(env, i + delta, patch, cnt);
> + *patch++ = BPF_ST_NOSPEC();
> + *patch++ = *insn;
> + cnt = patch - insn_buf;
> + new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, cnt);
> if (!new_prog)
> return -ENOMEM;
>
> @@ -21269,13 +21271,12 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
> /* nospec_result is only used to mitigate Spectre v4 and
> * to limit verification-time for Spectre v1.
> */
> - struct bpf_insn patch[] = {
> - *insn,
> - BPF_ST_NOSPEC(),
> - };
> + struct bpf_insn *patch = &insn_buf[0];
>
> - cnt = ARRAY_SIZE(patch);
> - new_prog = bpf_patch_insn_data(env, i + delta, patch, cnt);
> + *patch++ = *insn;
> + *patch++ = BPF_ST_NOSPEC();
> + cnt = patch - insn_buf;
> + new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, cnt);
> if (!new_prog)
> return -ENOMEM;
>
> @@ -21945,13 +21946,12 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
> u16 stack_depth_extra = 0;
>
> if (env->seen_exception && !env->exception_callback_subprog) {
> - struct bpf_insn patch[] = {
> - env->prog->insnsi[insn_cnt - 1],
> - BPF_MOV64_REG(BPF_REG_0, BPF_REG_1),
> - BPF_EXIT_INSN(),
> - };
> + struct bpf_insn *patch = &insn_buf[0];
>
> - ret = add_hidden_subprog(env, patch, ARRAY_SIZE(patch));
> + *patch++ = env->prog->insnsi[insn_cnt - 1];
> + *patch++ = BPF_MOV64_REG(BPF_REG_0, BPF_REG_1);
> + *patch++ = BPF_EXIT_INSN();
> + ret = add_hidden_subprog(env, insn_buf, patch - insn_buf);
> if (ret < 0)
> return ret;
> prog = env->prog;
> @@ -21987,20 +21987,18 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
> insn->off == 1 && insn->imm == -1) {
> bool is64 = BPF_CLASS(insn->code) == BPF_ALU64;
> bool isdiv = BPF_OP(insn->code) == BPF_DIV;
> - struct bpf_insn *patchlet;
> - struct bpf_insn chk_and_sdiv[] = {
> - BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
> - BPF_NEG | BPF_K, insn->dst_reg,
> - 0, 0, 0),
> - };
> - struct bpf_insn chk_and_smod[] = {
> - BPF_MOV32_IMM(insn->dst_reg, 0),
> - };
> + struct bpf_insn *patch = &insn_buf[0];
> +
> + if (isdiv)
> + *patch++ = BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
> + BPF_NEG | BPF_K, insn->dst_reg,
> + 0, 0, 0);
> + else
> + *patch++ = BPF_MOV32_IMM(insn->dst_reg, 0);
>
> - patchlet = isdiv ? chk_and_sdiv : chk_and_smod;
> - cnt = isdiv ? ARRAY_SIZE(chk_and_sdiv) : ARRAY_SIZE(chk_and_smod);
> + cnt = patch - insn_buf;
>
> - new_prog = bpf_patch_insn_data(env, i + delta, patchlet, cnt);
> + new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, cnt);
> if (!new_prog)
> return -ENOMEM;
>
> @@ -22019,83 +22017,73 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
> bool isdiv = BPF_OP(insn->code) == BPF_DIV;
> bool is_sdiv = isdiv && insn->off == 1;
> bool is_smod = !isdiv && insn->off == 1;
> - struct bpf_insn *patchlet;
> - struct bpf_insn chk_and_div[] = {
> - /* [R,W]x div 0 -> 0 */
> - BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
> - BPF_JNE | BPF_K, insn->src_reg,
> - 0, 2, 0),
> - BPF_ALU32_REG(BPF_XOR, insn->dst_reg, insn->dst_reg),
> - BPF_JMP_IMM(BPF_JA, 0, 0, 1),
> - *insn,
> - };
> - struct bpf_insn chk_and_mod[] = {
> - /* [R,W]x mod 0 -> [R,W]x */
> - BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
> - BPF_JEQ | BPF_K, insn->src_reg,
> - 0, 1 + (is64 ? 0 : 1), 0),
> - *insn,
> - BPF_JMP_IMM(BPF_JA, 0, 0, 1),
> - BPF_MOV32_REG(insn->dst_reg, insn->dst_reg),
> - };
> - struct bpf_insn chk_and_sdiv[] = {
> + struct bpf_insn *patch = &insn_buf[0];
> +
> + if (is_sdiv) {
> /* [R,W]x sdiv 0 -> 0
> * LLONG_MIN sdiv -1 -> LLONG_MIN
> * INT_MIN sdiv -1 -> INT_MIN
> */
> - BPF_MOV64_REG(BPF_REG_AX, insn->src_reg),
> - BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
> - BPF_ADD | BPF_K, BPF_REG_AX,
> - 0, 0, 1),
> - BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
> - BPF_JGT | BPF_K, BPF_REG_AX,
> - 0, 4, 1),
> - BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
> - BPF_JEQ | BPF_K, BPF_REG_AX,
> - 0, 1, 0),
> - BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
> - BPF_MOV | BPF_K, insn->dst_reg,
> - 0, 0, 0),
> + *patch++ = BPF_MOV64_REG(BPF_REG_AX, insn->src_reg);
> + *patch++ = BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
> + BPF_ADD | BPF_K, BPF_REG_AX,
> + 0, 0, 1);
> + *patch++ = BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
> + BPF_JGT | BPF_K, BPF_REG_AX,
> + 0, 4, 1);
> + *patch++ = BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
> + BPF_JEQ | BPF_K, BPF_REG_AX,
> + 0, 1, 0);
> + *patch++ = BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
> + BPF_MOV | BPF_K, insn->dst_reg,
> + 0, 0, 0);
> /* BPF_NEG(LLONG_MIN) == -LLONG_MIN == LLONG_MIN */
> - BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
> - BPF_NEG | BPF_K, insn->dst_reg,
> - 0, 0, 0),
> - BPF_JMP_IMM(BPF_JA, 0, 0, 1),
> - *insn,
> - };
> - struct bpf_insn chk_and_smod[] = {
> + *patch++ = BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
> + BPF_NEG | BPF_K, insn->dst_reg,
> + 0, 0, 0);
> + *patch++ = BPF_JMP_IMM(BPF_JA, 0, 0, 1);
> + *patch++ = *insn;
> + cnt = patch - insn_buf;
> + } else if (is_smod) {
> /* [R,W]x mod 0 -> [R,W]x */
> /* [R,W]x mod -1 -> 0 */
> - BPF_MOV64_REG(BPF_REG_AX, insn->src_reg),
> - BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
> - BPF_ADD | BPF_K, BPF_REG_AX,
> - 0, 0, 1),
> - BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
> - BPF_JGT | BPF_K, BPF_REG_AX,
> - 0, 3, 1),
> - BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
> - BPF_JEQ | BPF_K, BPF_REG_AX,
> - 0, 3 + (is64 ? 0 : 1), 1),
> - BPF_MOV32_IMM(insn->dst_reg, 0),
> - BPF_JMP_IMM(BPF_JA, 0, 0, 1),
> - *insn,
> - BPF_JMP_IMM(BPF_JA, 0, 0, 1),
> - BPF_MOV32_REG(insn->dst_reg, insn->dst_reg),
> - };
> -
> - if (is_sdiv) {
> - patchlet = chk_and_sdiv;
> - cnt = ARRAY_SIZE(chk_and_sdiv);
> - } else if (is_smod) {
> - patchlet = chk_and_smod;
> - cnt = ARRAY_SIZE(chk_and_smod) - (is64 ? 2 : 0);
> + *patch++ = BPF_MOV64_REG(BPF_REG_AX, insn->src_reg);
> + *patch++ = BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
> + BPF_ADD | BPF_K, BPF_REG_AX,
> + 0, 0, 1);
> + *patch++ = BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
> + BPF_JGT | BPF_K, BPF_REG_AX,
> + 0, 3, 1);
> + *patch++ = BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
> + BPF_JEQ | BPF_K, BPF_REG_AX,
> + 0, 3 + (is64 ? 0 : 1), 1);
> + *patch++ = BPF_MOV32_IMM(insn->dst_reg, 0);
> + *patch++ = BPF_JMP_IMM(BPF_JA, 0, 0, 1);
> + *patch++ = *insn;
> + *patch++ = BPF_JMP_IMM(BPF_JA, 0, 0, 1);
> + *patch++ = BPF_MOV32_REG(insn->dst_reg, insn->dst_reg);
> + cnt = (patch - insn_buf) - (is64 ? 2 : 0);
> + } else if (isdiv) {
> + /* [R,W]x div 0 -> 0 */
> + *patch++ = BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
> + BPF_JNE | BPF_K, insn->src_reg,
> + 0, 2, 0);
> + *patch++ = BPF_ALU32_REG(BPF_XOR, insn->dst_reg, insn->dst_reg);
> + *patch++ = BPF_JMP_IMM(BPF_JA, 0, 0, 1);
> + *patch++ = *insn;
> + cnt = patch - insn_buf;
> } else {
> - patchlet = isdiv ? chk_and_div : chk_and_mod;
> - cnt = isdiv ? ARRAY_SIZE(chk_and_div) :
> - ARRAY_SIZE(chk_and_mod) - (is64 ? 2 : 0);
> + /* [R,W]x mod 0 -> [R,W]x */
> + *patch++ = BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
> + BPF_JEQ | BPF_K, insn->src_reg,
> + 0, 1 + (is64 ? 0 : 1), 0);
> + *patch++ = *insn;
> + *patch++ = BPF_JMP_IMM(BPF_JA, 0, 0, 1);
> + *patch++ = BPF_MOV32_REG(insn->dst_reg, insn->dst_reg);
> + cnt = (patch - insn_buf) - (is64 ? 2 : 0);
> }
>
> - new_prog = bpf_patch_insn_data(env, i + delta, patchlet, cnt);
> + new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, cnt);
> if (!new_prog)
> return -ENOMEM;
>
> --
> 2.47.1
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: Reduce stack frame size by using env->insn_buf for bpf insns
2025-07-02 5:33 [PATCH bpf-next 1/2] bpf: Reduce stack frame size by using env->insn_buf for bpf insns Yonghong Song
2025-07-02 5:33 ` [PATCH bpf-next 2/2] bpf: Avoid putting struct bpf_scc_callchain variables on the stack Yonghong Song
2025-07-02 8:54 ` [PATCH bpf-next 1/2] bpf: Reduce stack frame size by using env->insn_buf for bpf insns Jiri Olsa
@ 2025-07-02 9:28 ` Arnd Bergmann
2025-07-02 15:09 ` Alexei Starovoitov
3 siblings, 0 replies; 7+ messages in thread
From: Arnd Bergmann @ 2025-07-02 9:28 UTC (permalink / raw)
To: Yonghong Song, bpf
Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, kernel-team,
Martin KaFai Lau
On Wed, Jul 2, 2025, at 07:33, Yonghong Song wrote:
> Arnd Bergmann reported an issue ([1]) where clang compiler (less than
> llvm18) may trigger an error where the stack frame size exceeds the
> limit.
> I can reproduce the error like below:
> kernel/bpf/verifier.c:24491:5: error: stack frame size (2552) exceeds
> limit (1280) in 'bpf_check'
> [-Werror,-Wframe-larger-than]
> kernel/bpf/verifier.c:19921:12: error: stack frame size (1368)
> exceeds limit (1280) in 'do_check'
> [-Werror,-Wframe-larger-than]
>
> Use env->insn_buf for bpf insns instead of putting these insns on the
> stack. This can resolve the above 'bpf_check' error. The 'do_check' error
> will be resolved in the next patch.
>
> [1] https://lore.kernel.org/bpf/20250620113846.3950478-1-arnd@kernel.org/
>
> Reported-by: Arnd Bergmann <arnd@kernel.org>
> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
I have confirmed that the fix addresses the issue in bpf_check().
In the worst case I see on an arm64 (clang-15, kasan) randconfigs,
the bpf_stack usage goes down from 1680 bytes to 1024.
On powerpc64 (clang-15, allmodconfig, kasan), I see an even larger
reduction from from 2112 bytes to 1200 for bpf_check().
Tested-by: Arnd Bergmann <arnd@arndb.de>
However, I still see 1952 bytes used in do_check() on powerpc64
allmodconfig. This number is much lower in newer clang versions,
I see 1888 bytes for clang-19 but only 1232 bytes for clang-20
and -21. Roughly a third of the 1952 bytes seems to come from
each one of do_check_insn() and is_state_visited(), but I haven't
found where exactly the stack is consumed there. This may be
a powerpc specific issue since on arm64 the same function needs
less than 1KB stack space.
I also tried turning off individual sanitizers (kasan, array-bounts,
trace-pc, ...) and they each seem to have a notable impact on
the total stack usage of powerpc64 do_check(), i.e. it's
not just a problem triggered by kasan or one particular other
option.
Arnd
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: Reduce stack frame size by using env->insn_buf for bpf insns
2025-07-02 5:33 [PATCH bpf-next 1/2] bpf: Reduce stack frame size by using env->insn_buf for bpf insns Yonghong Song
` (2 preceding siblings ...)
2025-07-02 9:28 ` Arnd Bergmann
@ 2025-07-02 15:09 ` Alexei Starovoitov
2025-07-02 15:29 ` Yonghong Song
3 siblings, 1 reply; 7+ messages in thread
From: Alexei Starovoitov @ 2025-07-02 15:09 UTC (permalink / raw)
To: Yonghong Song
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Kernel Team, Martin KaFai Lau, Arnd Bergmann
On Tue, Jul 1, 2025 at 10:33 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>
> Arnd Bergmann reported an issue ([1]) where clang compiler (less than
> llvm18) may trigger an error where the stack frame size exceeds the limit.
> I can reproduce the error like below:
> kernel/bpf/verifier.c:24491:5: error: stack frame size (2552) exceeds limit (1280) in 'bpf_check'
> [-Werror,-Wframe-larger-than]
> kernel/bpf/verifier.c:19921:12: error: stack frame size (1368) exceeds limit (1280) in 'do_check'
> [-Werror,-Wframe-larger-than]
>
> Use env->insn_buf for bpf insns instead of putting these insns on the
> stack. This can resolve the above 'bpf_check' error. The 'do_check' error
> will be resolved in the next patch.
>
> [1] https://lore.kernel.org/bpf/20250620113846.3950478-1-arnd@kernel.org/
>
> Reported-by: Arnd Bergmann <arnd@kernel.org>
> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
> ---
> kernel/bpf/verifier.c | 194 ++++++++++++++++++++----------------------
> 1 file changed, 91 insertions(+), 103 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 90e688f81a48..29faef51065d 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -20939,26 +20939,27 @@ static bool insn_is_cond_jump(u8 code)
> static void opt_hard_wire_dead_code_branches(struct bpf_verifier_env *env)
> {
> struct bpf_insn_aux_data *aux_data = env->insn_aux_data;
> - struct bpf_insn ja = BPF_JMP_IMM(BPF_JA, 0, 0, 0);
> + struct bpf_insn *ja = env->insn_buf;
This one replaces 8 bytes on stack with an 8 byte pointer.
Does it actually make a difference in stack usage?
I have my doubts.
> struct bpf_insn *insn = env->prog->insnsi;
> const int insn_cnt = env->prog->len;
> int i;
>
> + *ja = BPF_JMP_IMM(BPF_JA, 0, 0, 0);
> for (i = 0; i < insn_cnt; i++, insn++) {
> if (!insn_is_cond_jump(insn->code))
> continue;
>
> if (!aux_data[i + 1].seen)
> - ja.off = insn->off;
> + ja->off = insn->off;
> else if (!aux_data[i + 1 + insn->off].seen)
> - ja.off = 0;
> + ja->off = 0;
> else
> continue;
>
> if (bpf_prog_is_offloaded(env->prog->aux))
> - bpf_prog_offload_replace_insn(env, i, &ja);
> + bpf_prog_offload_replace_insn(env, i, ja);
>
> - memcpy(insn, &ja, sizeof(ja));
> + memcpy(insn, ja, sizeof(*ja));
> }
> }
>
> @@ -21017,7 +21018,9 @@ static int opt_remove_nops(struct bpf_verifier_env *env)
> static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env,
> const union bpf_attr *attr)
> {
> - struct bpf_insn *patch, zext_patch[2], rnd_hi32_patch[4];
> + struct bpf_insn *patch;
> + struct bpf_insn *zext_patch = env->insn_buf;
> + struct bpf_insn *rnd_hi32_patch = &env->insn_buf[2];
> struct bpf_insn_aux_data *aux = env->insn_aux_data;
> int i, patch_len, delta = 0, len = env->prog->len;
> struct bpf_insn *insns = env->prog->insnsi;
> @@ -21195,13 +21198,12 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
>
> if (env->insn_aux_data[i + delta].nospec) {
> WARN_ON_ONCE(env->insn_aux_data[i + delta].alu_state);
> - struct bpf_insn patch[] = {
> - BPF_ST_NOSPEC(),
> - *insn,
> - };
> + struct bpf_insn *patch = &insn_buf[0];
why &..[0] ? Can it just be patch = insn_buf ?
>
> - cnt = ARRAY_SIZE(patch);
> - new_prog = bpf_patch_insn_data(env, i + delta, patch, cnt);
> + *patch++ = BPF_ST_NOSPEC();
> + *patch++ = *insn;
> + cnt = patch - insn_buf;
> + new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, cnt);
> if (!new_prog)
> return -ENOMEM;
>
> @@ -21269,13 +21271,12 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
> /* nospec_result is only used to mitigate Spectre v4 and
> * to limit verification-time for Spectre v1.
> */
> - struct bpf_insn patch[] = {
> - *insn,
> - BPF_ST_NOSPEC(),
> - };
> + struct bpf_insn *patch = &insn_buf[0];
>
> - cnt = ARRAY_SIZE(patch);
> - new_prog = bpf_patch_insn_data(env, i + delta, patch, cnt);
> + *patch++ = *insn;
> + *patch++ = BPF_ST_NOSPEC();
> + cnt = patch - insn_buf;
> + new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, cnt);
> if (!new_prog)
> return -ENOMEM;
>
> @@ -21945,13 +21946,12 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
> u16 stack_depth_extra = 0;
>
> if (env->seen_exception && !env->exception_callback_subprog) {
> - struct bpf_insn patch[] = {
> - env->prog->insnsi[insn_cnt - 1],
> - BPF_MOV64_REG(BPF_REG_0, BPF_REG_1),
> - BPF_EXIT_INSN(),
> - };
> + struct bpf_insn *patch = &insn_buf[0];
>
> - ret = add_hidden_subprog(env, patch, ARRAY_SIZE(patch));
> + *patch++ = env->prog->insnsi[insn_cnt - 1];
> + *patch++ = BPF_MOV64_REG(BPF_REG_0, BPF_REG_1);
> + *patch++ = BPF_EXIT_INSN();
> + ret = add_hidden_subprog(env, insn_buf, patch - insn_buf);
> if (ret < 0)
> return ret;
> prog = env->prog;
> @@ -21987,20 +21987,18 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
> insn->off == 1 && insn->imm == -1) {
> bool is64 = BPF_CLASS(insn->code) == BPF_ALU64;
> bool isdiv = BPF_OP(insn->code) == BPF_DIV;
> - struct bpf_insn *patchlet;
> - struct bpf_insn chk_and_sdiv[] = {
> - BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
> - BPF_NEG | BPF_K, insn->dst_reg,
> - 0, 0, 0),
> - };
> - struct bpf_insn chk_and_smod[] = {
> - BPF_MOV32_IMM(insn->dst_reg, 0),
> - };
> + struct bpf_insn *patch = &insn_buf[0];
> +
> + if (isdiv)
> + *patch++ = BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
> + BPF_NEG | BPF_K, insn->dst_reg,
> + 0, 0, 0);
> + else
> + *patch++ = BPF_MOV32_IMM(insn->dst_reg, 0);
>
> - patchlet = isdiv ? chk_and_sdiv : chk_and_smod;
> - cnt = isdiv ? ARRAY_SIZE(chk_and_sdiv) : ARRAY_SIZE(chk_and_smod);
> + cnt = patch - insn_buf;
>
> - new_prog = bpf_patch_insn_data(env, i + delta, patchlet, cnt);
> + new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, cnt);
> if (!new_prog)
> return -ENOMEM;
>
> @@ -22019,83 +22017,73 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
> bool isdiv = BPF_OP(insn->code) == BPF_DIV;
> bool is_sdiv = isdiv && insn->off == 1;
> bool is_smod = !isdiv && insn->off == 1;
> - struct bpf_insn *patchlet;
> - struct bpf_insn chk_and_div[] = {
> - /* [R,W]x div 0 -> 0 */
> - BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
> - BPF_JNE | BPF_K, insn->src_reg,
> - 0, 2, 0),
> - BPF_ALU32_REG(BPF_XOR, insn->dst_reg, insn->dst_reg),
> - BPF_JMP_IMM(BPF_JA, 0, 0, 1),
> - *insn,
> - };
> - struct bpf_insn chk_and_mod[] = {
> - /* [R,W]x mod 0 -> [R,W]x */
> - BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
> - BPF_JEQ | BPF_K, insn->src_reg,
> - 0, 1 + (is64 ? 0 : 1), 0),
> - *insn,
> - BPF_JMP_IMM(BPF_JA, 0, 0, 1),
> - BPF_MOV32_REG(insn->dst_reg, insn->dst_reg),
> - };
> - struct bpf_insn chk_and_sdiv[] = {
> + struct bpf_insn *patch = &insn_buf[0];
> +
> + if (is_sdiv) {
> /* [R,W]x sdiv 0 -> 0
> * LLONG_MIN sdiv -1 -> LLONG_MIN
> * INT_MIN sdiv -1 -> INT_MIN
> */
> - BPF_MOV64_REG(BPF_REG_AX, insn->src_reg),
> - BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
> - BPF_ADD | BPF_K, BPF_REG_AX,
> - 0, 0, 1),
> - BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
> - BPF_JGT | BPF_K, BPF_REG_AX,
> - 0, 4, 1),
> - BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
> - BPF_JEQ | BPF_K, BPF_REG_AX,
> - 0, 1, 0),
> - BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
> - BPF_MOV | BPF_K, insn->dst_reg,
> - 0, 0, 0),
> + *patch++ = BPF_MOV64_REG(BPF_REG_AX, insn->src_reg);
> + *patch++ = BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
> + BPF_ADD | BPF_K, BPF_REG_AX,
> + 0, 0, 1);
> + *patch++ = BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
> + BPF_JGT | BPF_K, BPF_REG_AX,
> + 0, 4, 1);
> + *patch++ = BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
> + BPF_JEQ | BPF_K, BPF_REG_AX,
> + 0, 1, 0);
> + *patch++ = BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
> + BPF_MOV | BPF_K, insn->dst_reg,
> + 0, 0, 0);
> /* BPF_NEG(LLONG_MIN) == -LLONG_MIN == LLONG_MIN */
> - BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
> - BPF_NEG | BPF_K, insn->dst_reg,
> - 0, 0, 0),
> - BPF_JMP_IMM(BPF_JA, 0, 0, 1),
> - *insn,
> - };
> - struct bpf_insn chk_and_smod[] = {
> + *patch++ = BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
> + BPF_NEG | BPF_K, insn->dst_reg,
> + 0, 0, 0);
> + *patch++ = BPF_JMP_IMM(BPF_JA, 0, 0, 1);
> + *patch++ = *insn;
> + cnt = patch - insn_buf;
> + } else if (is_smod) {
> /* [R,W]x mod 0 -> [R,W]x */
> /* [R,W]x mod -1 -> 0 */
> - BPF_MOV64_REG(BPF_REG_AX, insn->src_reg),
> - BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
> - BPF_ADD | BPF_K, BPF_REG_AX,
> - 0, 0, 1),
> - BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
> - BPF_JGT | BPF_K, BPF_REG_AX,
> - 0, 3, 1),
> - BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
> - BPF_JEQ | BPF_K, BPF_REG_AX,
> - 0, 3 + (is64 ? 0 : 1), 1),
> - BPF_MOV32_IMM(insn->dst_reg, 0),
> - BPF_JMP_IMM(BPF_JA, 0, 0, 1),
> - *insn,
> - BPF_JMP_IMM(BPF_JA, 0, 0, 1),
> - BPF_MOV32_REG(insn->dst_reg, insn->dst_reg),
> - };
> -
> - if (is_sdiv) {
> - patchlet = chk_and_sdiv;
> - cnt = ARRAY_SIZE(chk_and_sdiv);
> - } else if (is_smod) {
> - patchlet = chk_and_smod;
> - cnt = ARRAY_SIZE(chk_and_smod) - (is64 ? 2 : 0);
> + *patch++ = BPF_MOV64_REG(BPF_REG_AX, insn->src_reg);
> + *patch++ = BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
> + BPF_ADD | BPF_K, BPF_REG_AX,
> + 0, 0, 1);
> + *patch++ = BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
> + BPF_JGT | BPF_K, BPF_REG_AX,
> + 0, 3, 1);
> + *patch++ = BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
> + BPF_JEQ | BPF_K, BPF_REG_AX,
> + 0, 3 + (is64 ? 0 : 1), 1);
> + *patch++ = BPF_MOV32_IMM(insn->dst_reg, 0);
> + *patch++ = BPF_JMP_IMM(BPF_JA, 0, 0, 1);
> + *patch++ = *insn;
> + *patch++ = BPF_JMP_IMM(BPF_JA, 0, 0, 1);
> + *patch++ = BPF_MOV32_REG(insn->dst_reg, insn->dst_reg);
> + cnt = (patch - insn_buf) - (is64 ? 2 : 0);
> + } else if (isdiv) {
> + /* [R,W]x div 0 -> 0 */
> + *patch++ = BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
> + BPF_JNE | BPF_K, insn->src_reg,
> + 0, 2, 0);
> + *patch++ = BPF_ALU32_REG(BPF_XOR, insn->dst_reg, insn->dst_reg);
> + *patch++ = BPF_JMP_IMM(BPF_JA, 0, 0, 1);
> + *patch++ = *insn;
> + cnt = patch - insn_buf;
> } else {
> - patchlet = isdiv ? chk_and_div : chk_and_mod;
> - cnt = isdiv ? ARRAY_SIZE(chk_and_div) :
> - ARRAY_SIZE(chk_and_mod) - (is64 ? 2 : 0);
> + /* [R,W]x mod 0 -> [R,W]x */
> + *patch++ = BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
> + BPF_JEQ | BPF_K, insn->src_reg,
> + 0, 1 + (is64 ? 0 : 1), 0);
> + *patch++ = *insn;
> + *patch++ = BPF_JMP_IMM(BPF_JA, 0, 0, 1);
> + *patch++ = BPF_MOV32_REG(insn->dst_reg, insn->dst_reg);
> + cnt = (patch - insn_buf) - (is64 ? 2 : 0);
hmm. why populate two extra insn just to drop them ?
Don't add the last two insn instead.
I would also simplify the previous jmp32 vs jmp generation by
testing if (is64) once and at the end cnt = patch - insn_buf;
--
pw-bot: cr
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: Reduce stack frame size by using env->insn_buf for bpf insns
2025-07-02 15:09 ` Alexei Starovoitov
@ 2025-07-02 15:29 ` Yonghong Song
0 siblings, 0 replies; 7+ messages in thread
From: Yonghong Song @ 2025-07-02 15:29 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Kernel Team, Martin KaFai Lau, Arnd Bergmann
On 7/2/25 8:09 AM, Alexei Starovoitov wrote:
> On Tue, Jul 1, 2025 at 10:33 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>> Arnd Bergmann reported an issue ([1]) where clang compiler (less than
>> llvm18) may trigger an error where the stack frame size exceeds the limit.
>> I can reproduce the error like below:
>> kernel/bpf/verifier.c:24491:5: error: stack frame size (2552) exceeds limit (1280) in 'bpf_check'
>> [-Werror,-Wframe-larger-than]
>> kernel/bpf/verifier.c:19921:12: error: stack frame size (1368) exceeds limit (1280) in 'do_check'
>> [-Werror,-Wframe-larger-than]
>>
>> Use env->insn_buf for bpf insns instead of putting these insns on the
>> stack. This can resolve the above 'bpf_check' error. The 'do_check' error
>> will be resolved in the next patch.
>>
>> [1] https://lore.kernel.org/bpf/20250620113846.3950478-1-arnd@kernel.org/
>>
>> Reported-by: Arnd Bergmann <arnd@kernel.org>
>> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
>> ---
>> kernel/bpf/verifier.c | 194 ++++++++++++++++++++----------------------
>> 1 file changed, 91 insertions(+), 103 deletions(-)
>>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 90e688f81a48..29faef51065d 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -20939,26 +20939,27 @@ static bool insn_is_cond_jump(u8 code)
>> static void opt_hard_wire_dead_code_branches(struct bpf_verifier_env *env)
>> {
>> struct bpf_insn_aux_data *aux_data = env->insn_aux_data;
>> - struct bpf_insn ja = BPF_JMP_IMM(BPF_JA, 0, 0, 0);
>> + struct bpf_insn *ja = env->insn_buf;
> This one replaces 8 bytes on stack with an 8 byte pointer.
> Does it actually make a difference in stack usage?
> I have my doubts.
Probably no difference. I just made change here just to remove
*all* 'struct bpf_insn var/var[]' on the stack. But as you suggested,
I will remove this change.
>
>> struct bpf_insn *insn = env->prog->insnsi;
>> const int insn_cnt = env->prog->len;
>> int i;
>>
>> + *ja = BPF_JMP_IMM(BPF_JA, 0, 0, 0);
>> for (i = 0; i < insn_cnt; i++, insn++) {
>> if (!insn_is_cond_jump(insn->code))
>> continue;
>>
>> if (!aux_data[i + 1].seen)
>> - ja.off = insn->off;
>> + ja->off = insn->off;
>> else if (!aux_data[i + 1 + insn->off].seen)
>> - ja.off = 0;
>> + ja->off = 0;
>> else
>> continue;
>>
>> if (bpf_prog_is_offloaded(env->prog->aux))
>> - bpf_prog_offload_replace_insn(env, i, &ja);
>> + bpf_prog_offload_replace_insn(env, i, ja);
>>
>> - memcpy(insn, &ja, sizeof(ja));
>> + memcpy(insn, ja, sizeof(*ja));
>> }
>> }
>>
>> @@ -21017,7 +21018,9 @@ static int opt_remove_nops(struct bpf_verifier_env *env)
>> static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env,
>> const union bpf_attr *attr)
>> {
>> - struct bpf_insn *patch, zext_patch[2], rnd_hi32_patch[4];
>> + struct bpf_insn *patch;
>> + struct bpf_insn *zext_patch = env->insn_buf;
>> + struct bpf_insn *rnd_hi32_patch = &env->insn_buf[2];
>> struct bpf_insn_aux_data *aux = env->insn_aux_data;
>> int i, patch_len, delta = 0, len = env->prog->len;
>> struct bpf_insn *insns = env->prog->insnsi;
>> @@ -21195,13 +21198,12 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
>>
>> if (env->insn_aux_data[i + delta].nospec) {
>> WARN_ON_ONCE(env->insn_aux_data[i + delta].alu_state);
>> - struct bpf_insn patch[] = {
>> - BPF_ST_NOSPEC(),
>> - *insn,
>> - };
>> + struct bpf_insn *patch = &insn_buf[0];
> why &..[0] ? Can it just be patch = insn_buf ?
There are a couple of cases in the existing code, e.g.,
$ grep -C 2 "&insn_buf\[0\]" verifier.c
(BPF_MODE(insn->code) == BPF_PROBE_MEM ||
BPF_MODE(insn->code) == BPF_PROBE_MEMSX)) {
struct bpf_insn *patch = &insn_buf[0];
u64 uaddress_limit = bpf_arch_uaddress_limit();
--
const u8 code_add = BPF_ALU64 | BPF_ADD | BPF_X;
const u8 code_sub = BPF_ALU64 | BPF_SUB | BPF_X;
struct bpf_insn *patch = &insn_buf[0];
bool issrc, isneg, isimm;
u32 off_reg;
I was using '... = &insn_buf[0]' to make code consistent.
Will change to use '... = insn_buf' including the above two cases.
>
>> - cnt = ARRAY_SIZE(patch);
>> - new_prog = bpf_patch_insn_data(env, i + delta, patch, cnt);
>> + *patch++ = BPF_ST_NOSPEC();
>> + *patch++ = *insn;
>> + cnt = patch - insn_buf;
>> + new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, cnt);
>> if (!new_prog)
>> return -ENOMEM;
>>
>> @@ -21269,13 +21271,12 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
>> /* nospec_result is only used to mitigate Spectre v4 and
>> * to limit verification-time for Spectre v1.
>> */
>> - struct bpf_insn patch[] = {
>> - *insn,
>> - BPF_ST_NOSPEC(),
>> - };
>> + struct bpf_insn *patch = &insn_buf[0];
>>
>> - cnt = ARRAY_SIZE(patch);
>> - new_prog = bpf_patch_insn_data(env, i + delta, patch, cnt);
>> + *patch++ = *insn;
>> + *patch++ = BPF_ST_NOSPEC();
>> + cnt = patch - insn_buf;
>> + new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, cnt);
>> if (!new_prog)
>> return -ENOMEM;
>>
>> @@ -21945,13 +21946,12 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
>> u16 stack_depth_extra = 0;
>>
>> if (env->seen_exception && !env->exception_callback_subprog) {
>> - struct bpf_insn patch[] = {
>> - env->prog->insnsi[insn_cnt - 1],
>> - BPF_MOV64_REG(BPF_REG_0, BPF_REG_1),
>> - BPF_EXIT_INSN(),
>> - };
>> + struct bpf_insn *patch = &insn_buf[0];
>>
>> - ret = add_hidden_subprog(env, patch, ARRAY_SIZE(patch));
>> + *patch++ = env->prog->insnsi[insn_cnt - 1];
>> + *patch++ = BPF_MOV64_REG(BPF_REG_0, BPF_REG_1);
>> + *patch++ = BPF_EXIT_INSN();
>> + ret = add_hidden_subprog(env, insn_buf, patch - insn_buf);
>> if (ret < 0)
>> return ret;
>> prog = env->prog;
>> @@ -21987,20 +21987,18 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
>> insn->off == 1 && insn->imm == -1) {
>> bool is64 = BPF_CLASS(insn->code) == BPF_ALU64;
>> bool isdiv = BPF_OP(insn->code) == BPF_DIV;
>> - struct bpf_insn *patchlet;
>> - struct bpf_insn chk_and_sdiv[] = {
>> - BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
>> - BPF_NEG | BPF_K, insn->dst_reg,
>> - 0, 0, 0),
>> - };
>> - struct bpf_insn chk_and_smod[] = {
>> - BPF_MOV32_IMM(insn->dst_reg, 0),
>> - };
>> + struct bpf_insn *patch = &insn_buf[0];
>> +
>> + if (isdiv)
>> + *patch++ = BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
>> + BPF_NEG | BPF_K, insn->dst_reg,
>> + 0, 0, 0);
>> + else
>> + *patch++ = BPF_MOV32_IMM(insn->dst_reg, 0);
>>
>> - patchlet = isdiv ? chk_and_sdiv : chk_and_smod;
>> - cnt = isdiv ? ARRAY_SIZE(chk_and_sdiv) : ARRAY_SIZE(chk_and_smod);
>> + cnt = patch - insn_buf;
>>
>> - new_prog = bpf_patch_insn_data(env, i + delta, patchlet, cnt);
>> + new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, cnt);
>> if (!new_prog)
>> return -ENOMEM;
>>
>> @@ -22019,83 +22017,73 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
>> bool isdiv = BPF_OP(insn->code) == BPF_DIV;
>> bool is_sdiv = isdiv && insn->off == 1;
>> bool is_smod = !isdiv && insn->off == 1;
>> - struct bpf_insn *patchlet;
>> - struct bpf_insn chk_and_div[] = {
>> - /* [R,W]x div 0 -> 0 */
>> - BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
>> - BPF_JNE | BPF_K, insn->src_reg,
>> - 0, 2, 0),
>> - BPF_ALU32_REG(BPF_XOR, insn->dst_reg, insn->dst_reg),
>> - BPF_JMP_IMM(BPF_JA, 0, 0, 1),
>> - *insn,
>> - };
>> - struct bpf_insn chk_and_mod[] = {
>> - /* [R,W]x mod 0 -> [R,W]x */
>> - BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
>> - BPF_JEQ | BPF_K, insn->src_reg,
>> - 0, 1 + (is64 ? 0 : 1), 0),
>> - *insn,
>> - BPF_JMP_IMM(BPF_JA, 0, 0, 1),
>> - BPF_MOV32_REG(insn->dst_reg, insn->dst_reg),
>> - };
>> - struct bpf_insn chk_and_sdiv[] = {
>> + struct bpf_insn *patch = &insn_buf[0];
>> +
>> + if (is_sdiv) {
>> /* [R,W]x sdiv 0 -> 0
>> * LLONG_MIN sdiv -1 -> LLONG_MIN
>> * INT_MIN sdiv -1 -> INT_MIN
>> */
>> - BPF_MOV64_REG(BPF_REG_AX, insn->src_reg),
>> - BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
>> - BPF_ADD | BPF_K, BPF_REG_AX,
>> - 0, 0, 1),
>> - BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
>> - BPF_JGT | BPF_K, BPF_REG_AX,
>> - 0, 4, 1),
>> - BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
>> - BPF_JEQ | BPF_K, BPF_REG_AX,
>> - 0, 1, 0),
>> - BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
>> - BPF_MOV | BPF_K, insn->dst_reg,
>> - 0, 0, 0),
>> + *patch++ = BPF_MOV64_REG(BPF_REG_AX, insn->src_reg);
>> + *patch++ = BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
>> + BPF_ADD | BPF_K, BPF_REG_AX,
>> + 0, 0, 1);
>> + *patch++ = BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
>> + BPF_JGT | BPF_K, BPF_REG_AX,
>> + 0, 4, 1);
>> + *patch++ = BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
>> + BPF_JEQ | BPF_K, BPF_REG_AX,
>> + 0, 1, 0);
>> + *patch++ = BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
>> + BPF_MOV | BPF_K, insn->dst_reg,
>> + 0, 0, 0);
>> /* BPF_NEG(LLONG_MIN) == -LLONG_MIN == LLONG_MIN */
>> - BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
>> - BPF_NEG | BPF_K, insn->dst_reg,
>> - 0, 0, 0),
>> - BPF_JMP_IMM(BPF_JA, 0, 0, 1),
>> - *insn,
>> - };
>> - struct bpf_insn chk_and_smod[] = {
>> + *patch++ = BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
>> + BPF_NEG | BPF_K, insn->dst_reg,
>> + 0, 0, 0);
>> + *patch++ = BPF_JMP_IMM(BPF_JA, 0, 0, 1);
>> + *patch++ = *insn;
>> + cnt = patch - insn_buf;
>> + } else if (is_smod) {
>> /* [R,W]x mod 0 -> [R,W]x */
>> /* [R,W]x mod -1 -> 0 */
>> - BPF_MOV64_REG(BPF_REG_AX, insn->src_reg),
>> - BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
>> - BPF_ADD | BPF_K, BPF_REG_AX,
>> - 0, 0, 1),
>> - BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
>> - BPF_JGT | BPF_K, BPF_REG_AX,
>> - 0, 3, 1),
>> - BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
>> - BPF_JEQ | BPF_K, BPF_REG_AX,
>> - 0, 3 + (is64 ? 0 : 1), 1),
>> - BPF_MOV32_IMM(insn->dst_reg, 0),
>> - BPF_JMP_IMM(BPF_JA, 0, 0, 1),
>> - *insn,
>> - BPF_JMP_IMM(BPF_JA, 0, 0, 1),
>> - BPF_MOV32_REG(insn->dst_reg, insn->dst_reg),
>> - };
>> -
>> - if (is_sdiv) {
>> - patchlet = chk_and_sdiv;
>> - cnt = ARRAY_SIZE(chk_and_sdiv);
>> - } else if (is_smod) {
>> - patchlet = chk_and_smod;
>> - cnt = ARRAY_SIZE(chk_and_smod) - (is64 ? 2 : 0);
>> + *patch++ = BPF_MOV64_REG(BPF_REG_AX, insn->src_reg);
>> + *patch++ = BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
>> + BPF_ADD | BPF_K, BPF_REG_AX,
>> + 0, 0, 1);
>> + *patch++ = BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
>> + BPF_JGT | BPF_K, BPF_REG_AX,
>> + 0, 3, 1);
>> + *patch++ = BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
>> + BPF_JEQ | BPF_K, BPF_REG_AX,
>> + 0, 3 + (is64 ? 0 : 1), 1);
>> + *patch++ = BPF_MOV32_IMM(insn->dst_reg, 0);
>> + *patch++ = BPF_JMP_IMM(BPF_JA, 0, 0, 1);
>> + *patch++ = *insn;
>> + *patch++ = BPF_JMP_IMM(BPF_JA, 0, 0, 1);
>> + *patch++ = BPF_MOV32_REG(insn->dst_reg, insn->dst_reg);
>> + cnt = (patch - insn_buf) - (is64 ? 2 : 0);
>> + } else if (isdiv) {
>> + /* [R,W]x div 0 -> 0 */
>> + *patch++ = BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
>> + BPF_JNE | BPF_K, insn->src_reg,
>> + 0, 2, 0);
>> + *patch++ = BPF_ALU32_REG(BPF_XOR, insn->dst_reg, insn->dst_reg);
>> + *patch++ = BPF_JMP_IMM(BPF_JA, 0, 0, 1);
>> + *patch++ = *insn;
>> + cnt = patch - insn_buf;
>> } else {
>> - patchlet = isdiv ? chk_and_div : chk_and_mod;
>> - cnt = isdiv ? ARRAY_SIZE(chk_and_div) :
>> - ARRAY_SIZE(chk_and_mod) - (is64 ? 2 : 0);
>> + /* [R,W]x mod 0 -> [R,W]x */
>> + *patch++ = BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
>> + BPF_JEQ | BPF_K, insn->src_reg,
>> + 0, 1 + (is64 ? 0 : 1), 0);
>> + *patch++ = *insn;
>> + *patch++ = BPF_JMP_IMM(BPF_JA, 0, 0, 1);
>> + *patch++ = BPF_MOV32_REG(insn->dst_reg, insn->dst_reg);
>> + cnt = (patch - insn_buf) - (is64 ? 2 : 0);
> hmm. why populate two extra insn just to drop them ?
> Don't add the last two insn instead.
> I would also simplify the previous jmp32 vs jmp generation by
> testing if (is64) once and at the end cnt = patch - insn_buf;
Make sense. Will do.
>
> --
> pw-bot: cr
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2025-07-02 15:29 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-02 5:33 [PATCH bpf-next 1/2] bpf: Reduce stack frame size by using env->insn_buf for bpf insns Yonghong Song
2025-07-02 5:33 ` [PATCH bpf-next 2/2] bpf: Avoid putting struct bpf_scc_callchain variables on the stack Yonghong Song
2025-07-02 8:49 ` Jiri Olsa
2025-07-02 8:54 ` [PATCH bpf-next 1/2] bpf: Reduce stack frame size by using env->insn_buf for bpf insns Jiri Olsa
2025-07-02 9:28 ` Arnd Bergmann
2025-07-02 15:09 ` Alexei Starovoitov
2025-07-02 15:29 ` Yonghong Song
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox