> bpf: Queue the rewrites of bpf_do_misc_fixups() and apply them at once
TL;DR: Possible regression in bpf_do_misc_fixups(): may_goto_expand() now sees
unpatched offsets, so a backward may_goto at exactly -32768 may get a 16-bit
JEQ instead of gotol, and a previously loading program may fail with -ERANGE.
> diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
> index 39566f382510..4b96f4ee9b3d 100644
> --- a/kernel/bpf/fixups.c
> +++ b/kernel/bpf/fixups.c
[ ... ]
> @@ -2336,13 +2316,9 @@ int bpf_do_misc_fixups(struct bpf_verifier_env *env)
> cnt = may_goto_expand(insn_buf, insn->off,
> stack_off_cnt,
> tail, ARRAY_SIZE(tail));
>
> - new_prog = bpf_patch_insn_data(env, i + delta,
> insn_buf, cnt);
> - if (!new_prog)
> - return -ENOMEM;
> -
> - delta += cnt - 1;
> - env->prog = prog = new_prog;
> - insn = new_prog->insnsi + i + delta;
> + insn = bpf_patch_list_add(env, i + delta, insn_buf,
> cnt);
> + if (IS_ERR(insn))
> + return PTR_ERR(insn);
> goto next_insn;
> } else if (bpf_is_may_goto_insn(insn)) {
> int stack_off = -stack_depth - 8;
> @@ -2355,13 +2331,9 @@ int bpf_do_misc_fixups(struct bpf_verifier_env *env)
> cnt = may_goto_expand(insn_buf, insn->off, stack_off,
> tail, ARRAY_SIZE(tail));
>
> - new_prog = bpf_patch_insn_data(env, i + delta,
> insn_buf, cnt);
> - if (!new_prog)
> - return -ENOMEM;
> -
> - delta += cnt - 1;
> - env->prog = prog = new_prog;
> - insn = new_prog->insnsi + i + delta;
> + insn = bpf_patch_list_add(env, i + delta, insn_buf,
> cnt);
> + if (IS_ERR(insn))
> + return PTR_ERR(insn);
> goto next_insn;
> }
>
Can this change make a program that loaded before get rejected at commit
time?
may_goto_expand() picks between a 16-bit JEQ and JNE + gotol based on the
offset it is given:
off = off >= 0 ? off + tail_cnt : off - 1;
...
if (off == (s16)off) {
insn_buf[cnt++] = BPF_JMP_IMM(BPF_JEQ, BPF_REG_AX, 0, off);
} else {
insn_buf[cnt++] = BPF_JMP_IMM(BPF_JNE, BPF_REG_AX, 0, 1);
insn_buf[cnt++] = BPF_JMP32_A(off >= 0 ? off : off - 1);
}
Before this change every earlier rewrite was applied right away through
bpf_patch_insn_data(), and bpf_adj_branches() updated the jumps after each
one. So when the loop reached a backward may_goto, insn->off already
included all the growth between the jump target and the may_goto. That is
the final backward distance, and the JEQ-or-gotol choice was made on it.
Now the loop walks the unpatched program, so insn->off is the original,
unpatched offset. The real distance is only computed later, in
patch_list_adj_insn() called from bpf_patch_list_commit():
rel = (s64)new_off[tgt] - pos - 1;
...
if (rel < S16_MIN || rel > S16_MAX)
return -ERANGE;
Take a backward may_goto whose grown offset is exactly -32768 (S16_MIN).
The old code saw off = -32768, computed off - 1 = -32769, which does not
fit in s16, and emitted JNE + gotol, so the program loaded. The new code
sees the smaller original offset, so off - 1 fits in s16 and it emits JEQ.
At commit the JEQ's displacement is -32769, patch_list_adj_insn() returns
-ERANGE, and the program is rejected with "insn %d cannot be patched due to
16-bit range".
The non-timed path under bpf_is_may_goto_insn(), which also calls
may_goto_expand(), has the same problem. Forward may_goto behaves as
before, since the old code also chose JEQ from the unpatched forward
offset. The trigger is narrow, a single exact displacement.
The selftest added later in the series (f8ef7933910d) does not appear to
cover this gotol boundary, and no later commit in the series changes it.
Should the JEQ-or-gotol decision be deferred until the final offsets are
known?
---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/36845462631
