Re: [PATCH bpf-next v2 12/22] bpf: Teach the patch list to insert in front of an instruction

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 2624 bytes --]

> bpf: Teach the patch list to insert in front of an instruction

TL;DR: Nit only: the updated header comment on bpf_patch_list_commit() says
everything follows the insn itself after an insertion, but only jumps, insn
arrays and poke descriptors do; the code and commit message differ.

> diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
> index a56b743be006..bb4fd75a7062 100644
> --- a/kernel/bpf/fixups.c
> +++ b/kernel/bpf/fixups.c

[ ... ]

> @@ -533,8 +557,9 @@ static void patch_list_del_subprogs(struct bpf_verifier_env *env,
>  /*
>   * Apply all queued patches and removals in one go. Everything that names an
>   * insn of the old image follows it to the new one. A patched insn is named by
> - * the first insn of its patch, a removed insn by the next insn that stays,
> - * like with bpf_patch_insn_data().
> + * the first insn of its patch, or the insn itself if insns were inserted in
> + * front of it, a removed insn by the next insn that stays, like with
> + * bpf_patch_insn_data().
>   */
>  int bpf_patch_list_commit(struct bpf_verifier_env *env)
>  {

This isn't a bug, but is the updated header comment accurate about which
users follow the insn itself after an insertion?

It says everything that names an old insn follows it, and that an insn with
insns inserted in front of it is named by the insn itself.  In
bpf_patch_list_commit() that only holds for jumps, insn arrays and poke
descriptors, which use img.new_tgt:

    bpf_insn_array_remap(env->insn_array_maps[i], img.new_off,
                         img.new_tgt, img.cnt, img.grow);

Subprog starts, func pointers and line info still use img.new_off, which is
the first inserted insn:

    env->subprog_info[i].start = bpf_patch_image_remap(&img, img.new_off, ...)
    fp->xlated_off = bpf_patch_image_remap(&img, img.new_off, fp->xlated_off)

patch_list_adj_insn() also sends calls and ld_imm64 of subprogs to new_off.

The commit message and the comment above __bpf_patch_list_insert() describe
this split correctly, so this comment now disagrees with them.  Also,
"like with bpf_patch_insn_data()" now trails a clause that
bpf_patch_insn_data() never implements, since it always uses
BPF_PATCH_KEEP_TARGET.

Could the comment say that jumps, instruction arrays and poke descriptors go
to the insn itself, while subprogs, line info, function pointers and calls go
to the first inserted insn?


---
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/41053659762