* [PATCH bpf-next v1 1/2] bpf: removed unused 'env' parameter from is_reg64 and insn_has_def32
@ 2025-08-06 20:09 Eduard Zingerman
2025-08-06 20:09 ` [PATCH bpf-next v1 2/2] bpf: use realloc in bpf_patch_insn_data Eduard Zingerman
0 siblings, 1 reply; 4+ messages in thread
From: Eduard Zingerman @ 2025-08-06 20:09 UTC (permalink / raw)
To: bpf, ast, andrii; +Cc: daniel, martin.lau, kernel-team, yonghong.song, eddyz87
Parameter 'env' is not used by is_reg64() and insn_has_def32()
functions. Remove the parameter to make it clear that neither function
depends on 'env' state, e.g. env->insn_aux_data.
Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
kernel/bpf/verifier.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 0806295945e4..69eb2b5c2218 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -3663,7 +3663,7 @@ static int mark_irq_flag_read(struct bpf_verifier_env *env, struct bpf_reg_state
* code only. It returns TRUE if the source or destination register operates
* on 64-bit, otherwise return FALSE.
*/
-static bool is_reg64(struct bpf_verifier_env *env, struct bpf_insn *insn,
+static bool is_reg64(struct bpf_insn *insn,
u32 regno, struct bpf_reg_state *reg, enum reg_arg_type t)
{
u8 code, class, op;
@@ -3774,14 +3774,14 @@ static int insn_def_regno(const struct bpf_insn *insn)
}
/* Return TRUE if INSN has defined any 32-bit value explicitly. */
-static bool insn_has_def32(struct bpf_verifier_env *env, struct bpf_insn *insn)
+static bool insn_has_def32(struct bpf_insn *insn)
{
int dst_reg = insn_def_regno(insn);
if (dst_reg == -1)
return false;
- return !is_reg64(env, insn, dst_reg, NULL, DST_OP);
+ return !is_reg64(insn, dst_reg, NULL, DST_OP);
}
static void mark_insn_zext(struct bpf_verifier_env *env,
@@ -3812,7 +3812,7 @@ static int __check_reg_arg(struct bpf_verifier_env *env, struct bpf_reg_state *r
mark_reg_scratched(env, regno);
reg = ®s[regno];
- rw64 = is_reg64(env, insn, regno, reg, t);
+ rw64 = is_reg64(insn, regno, reg, t);
if (t == SRC_OP) {
/* check whether register used as source operand can be read */
if (reg->type == NOT_INIT) {
@@ -20712,7 +20712,7 @@ static void adjust_insn_aux_data(struct bpf_verifier_env *env,
* (cnt == 1) is taken or not. There is no guarantee INSN at OFF is the
* original insn at old prog.
*/
- old_data[off].zext_dst = insn_has_def32(env, insn + off + cnt - 1);
+ old_data[off].zext_dst = insn_has_def32(insn + off + cnt - 1);
if (cnt == 1)
return;
@@ -20724,7 +20724,7 @@ static void adjust_insn_aux_data(struct bpf_verifier_env *env,
for (i = off; i < off + cnt - 1; i++) {
/* Expand insni[off]'s seen count to the patched range. */
new_data[i].seen = old_seen;
- new_data[i].zext_dst = insn_has_def32(env, insn + i);
+ new_data[i].zext_dst = insn_has_def32(insn + i);
}
env->insn_aux_data = new_data;
vfree(old_data);
@@ -21131,7 +21131,7 @@ static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env,
* BPF_STX + SRC_OP, so it is safe to pass NULL
* here.
*/
- if (is_reg64(env, &insn, load_reg, NULL, DST_OP)) {
+ if (is_reg64(&insn, load_reg, NULL, DST_OP)) {
if (class == BPF_LD &&
BPF_MODE(code) == BPF_IMM)
i++;
--
2.47.3
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH bpf-next v1 2/2] bpf: use realloc in bpf_patch_insn_data
2025-08-06 20:09 [PATCH bpf-next v1 1/2] bpf: removed unused 'env' parameter from is_reg64 and insn_has_def32 Eduard Zingerman
@ 2025-08-06 20:09 ` Eduard Zingerman
2025-08-06 23:04 ` Eduard Zingerman
0 siblings, 1 reply; 4+ messages in thread
From: Eduard Zingerman @ 2025-08-06 20:09 UTC (permalink / raw)
To: bpf, ast, andrii
Cc: daniel, martin.lau, kernel-team, yonghong.song, eddyz87,
Alexei Starovoitov
Avoid excessive vzalloc/vfree calls when patching instructions in
do_misc_fixups(). bpf_patch_insn_data() uses vzalloc to allocate new
memory for env->insn_aux_data for each patch as follows:
struct bpf_prog *bpf_patch_insn_data(env, ...)
{
...
new_data = vzalloc(... O(program size) ...);
...
adjust_insn_aux_data(env, new_data, ...);
...
}
void adjust_insn_aux_data(env, new_data, ...)
{
...
memcpy(new_data, env->insn_aux_data);
vfree(env->insn_aux_data);
env->insn_aux_data = new_data;
...
}
The vzalloc/vfree pair is hot in perf report collected for e.g.
pyperf180 test case. It can be replaced with a call to vrealloc in a
hope to reduce the number of actual memory allocations.
This is a stop-gap solution, as bpf_patch_insn_data is still hot in
the profile. More comprehansive solutions had been discussed before
e.g. as in [1].
Perf stat w/o this patch:
$ perf stat -B --all-kernel -r10 -- ./veristat -q pyperf180.bpf.o
...
2201.25 msec task-clock # 0.973 CPUs utilized ( +- 2.20% )
188 context-switches # 85.406 /sec ( +- 9.29% )
15 cpu-migrations # 6.814 /sec ( +- 5.64% )
5 page-faults # 2.271 /sec ( +- 3.27% )
4315057974 instructions # 1.28 insn per cycle
# 0.33 stalled cycles per insn ( +- 0.03% )
3366141387 cycles # 1.529 GHz ( +- 0.21% )
1420810964 stalled-cycles-frontend # 42.21% frontend cycles idle ( +- 0.23% )
1049956791 branches # 476.981 M/sec ( +- 0.03% )
60591781 branch-misses # 5.77% of all branches ( +- 0.07% )
2.2632 +- 0.0527 seconds time elapsed ( +- 2.33% )
Perf stat with this patch:
1132.77 msec task-clock # 0.976 CPUs utilized ( +- 3.47% )
80 context-switches # 70.623 /sec ( +- 31.57% )
1 cpu-migrations # 0.883 /sec ( +- 37.27% )
5 page-faults # 4.414 /sec ( +- 3.59% )
3307816503 instructions # 2.20 insn per cycle
# 0.15 stalled cycles per insn ( +- 0.05% )
1506171011 cycles # 1.330 GHz ( +- 0.55% )
488914539 stalled-cycles-frontend # 32.46% frontend cycles idle ( +- 0.94% )
729783557 branches # 644.246 M/sec ( +- 0.05% )
17312298 branch-misses # 2.37% of all branches ( +- 0.23% )
1.1602 +- 0.0443 seconds time elapsed ( +- 3.82% )
[1] https://lore.kernel.org/bpf/CAEf4BzY_E8MSL4mD0UPuuiDcbJhh9e2xQo2=5w+ppRWWiYSGvQ@mail.gmail.com/
Suggested-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
kernel/bpf/verifier.c | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 69eb2b5c2218..6ef7dc6079a4 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -20699,12 +20699,11 @@ static void convert_pseudo_ld_imm64(struct bpf_verifier_env *env)
* [0, off) and [off, end) to new locations, so the patched range stays zero
*/
static void adjust_insn_aux_data(struct bpf_verifier_env *env,
- struct bpf_insn_aux_data *new_data,
struct bpf_prog *new_prog, u32 off, u32 cnt)
{
- struct bpf_insn_aux_data *old_data = env->insn_aux_data;
+ struct bpf_insn_aux_data *data = env->insn_aux_data;
struct bpf_insn *insn = new_prog->insnsi;
- u32 old_seen = old_data[off].seen;
+ u32 old_seen = data[off].seen;
u32 prog_len;
int i;
@@ -20712,22 +20711,19 @@ static void adjust_insn_aux_data(struct bpf_verifier_env *env,
* (cnt == 1) is taken or not. There is no guarantee INSN at OFF is the
* original insn at old prog.
*/
- old_data[off].zext_dst = insn_has_def32(insn + off + cnt - 1);
+ data[off].zext_dst = insn_has_def32(insn + off + cnt - 1);
if (cnt == 1)
return;
prog_len = new_prog->len;
- memcpy(new_data, old_data, sizeof(struct bpf_insn_aux_data) * off);
- memcpy(new_data + off + cnt - 1, old_data + off,
- sizeof(struct bpf_insn_aux_data) * (prog_len - off - cnt + 1));
+ memmove(data + off + cnt - 1, data + off,
+ sizeof(struct bpf_insn_aux_data) * (prog_len - off - cnt + 1));
for (i = off; i < off + cnt - 1; i++) {
/* Expand insni[off]'s seen count to the patched range. */
- new_data[i].seen = old_seen;
- new_data[i].zext_dst = insn_has_def32(insn + i);
+ data[i].seen = old_seen;
+ data[i].zext_dst = insn_has_def32(insn + i);
}
- env->insn_aux_data = new_data;
- vfree(old_data);
}
static void adjust_subprog_starts(struct bpf_verifier_env *env, u32 off, u32 len)
@@ -20765,10 +20761,14 @@ static struct bpf_prog *bpf_patch_insn_data(struct bpf_verifier_env *env, u32 of
struct bpf_insn_aux_data *new_data = NULL;
if (len > 1) {
- new_data = vzalloc(array_size(env->prog->len + len - 1,
- sizeof(struct bpf_insn_aux_data)));
+ new_data = vrealloc(env->insn_aux_data,
+ array_size(env->prog->len + len - 1,
+ sizeof(struct bpf_insn_aux_data)),
+ GFP_KERNEL_ACCOUNT | __GFP_ZERO);
if (!new_data)
return NULL;
+
+ env->insn_aux_data = new_data;
}
new_prog = bpf_patch_insn_single(env->prog, off, patch, len);
@@ -20780,7 +20780,7 @@ static struct bpf_prog *bpf_patch_insn_data(struct bpf_verifier_env *env, u32 of
vfree(new_data);
return NULL;
}
- adjust_insn_aux_data(env, new_data, new_prog, off, len);
+ adjust_insn_aux_data(env, new_prog, off, len);
adjust_subprog_starts(env, off, len);
adjust_poke_descs(new_prog, off, len);
return new_prog;
--
2.47.3
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH bpf-next v1 2/2] bpf: use realloc in bpf_patch_insn_data
2025-08-06 20:09 ` [PATCH bpf-next v1 2/2] bpf: use realloc in bpf_patch_insn_data Eduard Zingerman
@ 2025-08-06 23:04 ` Eduard Zingerman
2025-08-06 23:54 ` Eduard Zingerman
0 siblings, 1 reply; 4+ messages in thread
From: Eduard Zingerman @ 2025-08-06 23:04 UTC (permalink / raw)
To: bpf, ast, andrii
Cc: daniel, martin.lau, kernel-team, yonghong.song,
Alexei Starovoitov
On Wed, 2025-08-06 at 13:09 -0700, Eduard Zingerman wrote:
[...]
> @@ -20712,22 +20711,19 @@ static void adjust_insn_aux_data(struct bpf_verifier_env *env,
> * (cnt == 1) is taken or not. There is no guarantee INSN at OFF is the
> * original insn at old prog.
> */
> - old_data[off].zext_dst = insn_has_def32(insn + off + cnt - 1);
> + data[off].zext_dst = insn_has_def32(insn + off + cnt - 1);
>
> if (cnt == 1)
> return;
> prog_len = new_prog->len;
>
> - memcpy(new_data, old_data, sizeof(struct bpf_insn_aux_data) * off);
> - memcpy(new_data + off + cnt - 1, old_data + off,
> - sizeof(struct bpf_insn_aux_data) * (prog_len - off - cnt + 1));
> + memmove(data + off + cnt - 1, data + off,
> + sizeof(struct bpf_insn_aux_data) * (prog_len - off - cnt + 1));
> for (i = off; i < off + cnt - 1; i++) {
> /* Expand insni[off]'s seen count to the patched range. */
> - new_data[i].seen = old_seen;
> - new_data[i].zext_dst = insn_has_def32(insn + i);
> + data[i].seen = old_seen;
> + data[i].zext_dst = insn_has_def32(insn + i);
> }
> - env->insn_aux_data = new_data;
> - vfree(old_data);
> }
veristat-meta job failed on the CI [1] because the following piece is missing:
@@ -20719,6 +20719,7 @@ static void adjust_insn_aux_data(struct bpf_verifier_env *env,
memmove(data + off + cnt - 1, data + off,
sizeof(struct bpf_insn_aux_data) * (prog_len - off - cnt + 1));
+ memset(data + off, 0, sizeof(struct bpf_insn_aux_data) * (cnt - 1));
for (i = off; i < off + cnt - 1; i++) {
/* Expand insni[off]'s seen count to the patched range. */
data[i].seen = old_seen;
I'm trying to figure out if I can add a selftest for this.
[1] https://github.com/kernel-patches/bpf/actions/runs/16787563163/job/47542309875
[...]
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH bpf-next v1 2/2] bpf: use realloc in bpf_patch_insn_data
2025-08-06 23:04 ` Eduard Zingerman
@ 2025-08-06 23:54 ` Eduard Zingerman
0 siblings, 0 replies; 4+ messages in thread
From: Eduard Zingerman @ 2025-08-06 23:54 UTC (permalink / raw)
To: bpf, ast, andrii
Cc: daniel, martin.lau, kernel-team, yonghong.song,
Alexei Starovoitov
On Wed, 2025-08-06 at 16:04 -0700, Eduard Zingerman wrote:
> On Wed, 2025-08-06 at 13:09 -0700, Eduard Zingerman wrote:
>
> [...]
>
> > @@ -20712,22 +20711,19 @@ static void adjust_insn_aux_data(struct bpf_verifier_env *env,
> > * (cnt == 1) is taken or not. There is no guarantee INSN at OFF is the
> > * original insn at old prog.
> > */
> > - old_data[off].zext_dst = insn_has_def32(insn + off + cnt - 1);
> > + data[off].zext_dst = insn_has_def32(insn + off + cnt - 1);
> >
> > if (cnt == 1)
> > return;
> > prog_len = new_prog->len;
> >
> > - memcpy(new_data, old_data, sizeof(struct bpf_insn_aux_data) * off);
> > - memcpy(new_data + off + cnt - 1, old_data + off,
> > - sizeof(struct bpf_insn_aux_data) * (prog_len - off - cnt + 1));
> > + memmove(data + off + cnt - 1, data + off,
> > + sizeof(struct bpf_insn_aux_data) * (prog_len - off - cnt + 1));
> > for (i = off; i < off + cnt - 1; i++) {
> > /* Expand insni[off]'s seen count to the patched range. */
> > - new_data[i].seen = old_seen;
> > - new_data[i].zext_dst = insn_has_def32(insn + i);
> > + data[i].seen = old_seen;
> > + data[i].zext_dst = insn_has_def32(insn + i);
> > }
> > - env->insn_aux_data = new_data;
> > - vfree(old_data);
> > }
>
> veristat-meta job failed on the CI [1] because the following piece is missing:
>
> @@ -20719,6 +20719,7 @@ static void adjust_insn_aux_data(struct bpf_verifier_env *env,
>
> memmove(data + off + cnt - 1, data + off,
> sizeof(struct bpf_insn_aux_data) * (prog_len - off - cnt + 1));
> + memset(data + off, 0, sizeof(struct bpf_insn_aux_data) * (cnt - 1));
> for (i = off; i < off + cnt - 1; i++) {
> /* Expand insni[off]'s seen count to the patched range. */
> data[i].seen = old_seen;
>
> I'm trying to figure out if I can add a selftest for this.
>
> [1] https://github.com/kernel-patches/bpf/actions/runs/16787563163/job/47542309875
>
> [...]
The error reported by verifier is "verifier bug: error during ctx access conversion (0)",
signaled from convert_ctx_accesses(). The rewrite is attempted because
`env->insn_aux_data[i + delta].ptr_type` is set to 12 (PTR_TO_SOCK_COMMON).
The instruction for which rewrite is attempted is a load or store
instruction introduced as a result of inline_bpf_loop() call.
It has a wrong offset for bpf_sock_convert_ctx_access() rewrite,
hence rewrite attempt is unsuccessful and the above mentioned error is reported.
`env->insn_aux_data[i + delta].ptr_type` is set for the instruction in question
because of missing memset(0). It is a value of the insn_aux_data inherited
from an instruction occurring at a small offset after bpf_loop call.
Here is a similar reproducer, but for PTR_TO_CTX (== 2):
struct { ... } map0 SEC(".maps"); // any valid map definition
struct { ... } map1 SEC(".maps");
struct { ... } map2 SEC(".maps");
SEC("xdp")
__success
__naked void bug1(void)
{
asm volatile (" \
r0 = %[map0] ll; /* 0 */ \
r0 = %[map1] ll; /* 2 */ \
r1 = 1; /* 4 */ \
r2 = dummy ll; /* 5 */ \
r3 = 0; /* 7 */ \
r4 = 0; /* 8 */ \
call %[bpf_loop]; /* 9 */ \
r0 = 0; /* 10 */ \
r0 = 0; /* 11 */ \
r0 = %[map2] ll; /* 12 */ \
exit; \
" :
: __imm(bpf_loop),
__imm_addr(map0),
__imm_addr(map1),
__imm_addr(map2)
: __clobber_all);
}
Instruction `call %[bpf_loop]` is replaced by a sequence:
9: if r1 <= 0x800000 goto pc+2
10: w0 = -7
11: goto pc+16
12: *(u64 *)(r10 -24) = r6
...
Note the store at offset (12). Because of the missing memset(0) it
inherits insn_aux_data fields from original instruction #12: `r0 = %[map2] ll`.
`struct bpf_insn_aux_data` is defined as follows:
struct bpf_insn_aux_data {
union {
enum bpf_reg_type ptr_type;
...
struct {
u32 map_index; /* index into used_maps[] */
...
};
...
};
}
Here fields .ptr_type and .map_index have same offset.
The example above forces convert_ctx_accesses() to interpret .map_index as a .ptr_type.
The .map_index at offset 12 happens to be 2, which corresponds to PTR_TO_CTX.
convert_ctx_accesses() attempts to rewrite `12: *(u64 *)(r10 -24) = r6` and fails.
All in all, I think this test is fragile, so I'll post v2 w/o a selftest.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-08-06 23:54 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-06 20:09 [PATCH bpf-next v1 1/2] bpf: removed unused 'env' parameter from is_reg64 and insn_has_def32 Eduard Zingerman
2025-08-06 20:09 ` [PATCH bpf-next v1 2/2] bpf: use realloc in bpf_patch_insn_data Eduard Zingerman
2025-08-06 23:04 ` Eduard Zingerman
2025-08-06 23:54 ` Eduard Zingerman
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.