Re: [PATCH bpf-next v2 05/22] bpf: Expand a backward may_goto to gotol when patches push it out of range

[bot+bpf-ci@kernel.org]

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

> bpf: Expand a backward may_goto to gotol when patches push it out of range

TL;DR: Nits only: the new may_goto_expand() comment on 'reach' overstates
coverage, since forward jumps are still unadjusted, and it detaches the older
function comment.

> diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
> index c9963d8d8135..1d281428acbb 100644
> --- a/kernel/bpf/fixups.c
> +++ b/kernel/bpf/fixups.c
> @@ -1968,16 +1968,21 @@ static int add_hidden_subprog(struct bpf_verifier_env *env, struct bpf_insn *pat
>   * may_goto when it is zero, then the tail that updates the count.
>   * Use gotol when the target is too far for 16-bit offset of a conditional jump.
>   */
> -static int may_goto_expand(struct bpf_insn *insn_buf, int off, int stack_off,
> -			   const struct bpf_insn *tail, int tail_cnt)
> +/*
> + * 'off' is the offset of may_goto, 'reach' how far it jumps once the patches
> + * in between are applied, which decides whether a 16-bit jump will do.
> + */
> +static int may_goto_expand(struct bpf_insn *insn_buf, int off, int reach,
> +			   int stack_off, const struct bpf_insn *tail, int tail_cnt)

This isn't a bug, but the new comment says 'reach' is how far may_goto jumps
once the patches in between are applied.  Does that hold for forward jumps?

may_goto_off() returns the unpatched offset for them:

	if (off >= 0)
		return off;

so reach == off for a forward may_goto, and a forward may_goto that later
patches push past S16_MAX still gets a 16-bit JEQ.  That then fails in
bpf_patch_list_commit() -> patch_list_adj_insn() -> insn_set_rel() with
-ERANGE, the same as before this patch.

Could the comment say that 'reach' only accounts for the patches already
queued, so only backward jumps are covered?

Also, the new comment block sits directly below the existing comment for
may_goto_expand(), which leaves the older comment detached from the function
it describes.  Could the two be merged into one?

>  {
>  	int cnt = 0;
>
>  	/* Forward jump has to step over the tail */
>  	off = off >= 0 ? off + tail_cnt : off - 1;
> +	reach = reach >= 0 ? reach + tail_cnt : reach - 1;
>
>  	insn_buf[cnt++] = BPF_LDX_MEM(BPF_DW, BPF_REG_AX, BPF_REG_10, stack_off);
> -	if (off == (s16)off) {
> +	if (reach == (s16)reach) {
>  		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);
> @@ -1987,6 +1992,23 @@ static int may_goto_expand(struct bpf_insn *insn_buf, int off, int stack_off,
>  	return cnt + tail_cnt;
>  }

[ ... ]


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