Re: [PATCH bpf-next v9 1/8] bpf: mark instructions accessing program stack

[bot+bpf-ci@kernel.org]

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

> diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
> index 65b441e4a3517..73fb3ffc18e33 100644
> --- a/kernel/bpf/fixups.c
> +++ b/kernel/bpf/fixups.c
> @@ -213,7 +213,8 @@ static int get_callee_stack_depth(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_prog *new_prog, u32 off, u32 cnt)
> +				 struct bpf_prog *new_prog, u32 off, u32 cnt,
> +				 struct bpf_insn *original_insn)
>  {
>  	struct bpf_insn_aux_data *data = env->insn_aux_data;
>  	struct bpf_insn *insn = new_prog->insnsi;
> @@ -227,8 +228,15 @@ static void adjust_insn_aux_data(struct bpf_verifier_env *env,
>  	 */
>  	data[off].zext_dst = bpf_insn_def32(new_prog, insn + off + cnt - 1) >= 0;
>
> -	if (cnt == 1)
> +	if (cnt == 1) {
> +		/*
> +		 * A non-memory accessing insn could have been replaced by a
> +		 * memory accessing insn, systematically mark it for non-stack
> +		 * access
> +		 */
> +		data[off].non_stack_access = bpf_is_mem_insn(insn + off);
>  		return;
> +	}

This isn't a bug, but does the comment explain why the marking needs to be
unconditionally reset here? The statement is an assignment that both sets
and clears the flag depending on the result, but the comment only mentions
the setting direction.

>  	prog_len = new_prog->len;
>  	env->insn_aux_data_len = prog_len;
>
> @@ -239,8 +247,25 @@ static void adjust_insn_aux_data(struct bpf_verifier_env *env,
>  		/* Expand insni[off]'s seen count to the patched range. */
>  		data[i].seen = old_seen;
>  		data[i].zext_dst = bpf_insn_def32(new_prog, insn + i) >= 0;
> +		if (!memcmp(insn + i, original_insn, sizeof(struct bpf_insn))) {
> +			data[i].non_stack_access =
> +				data[off + cnt - 1].non_stack_access;
> +			data[off + cnt - 1].non_stack_access = false;
> +		} else if (bpf_is_mem_insn(insn + i)) {
> +			data[i].non_stack_access = true;
> +		}

This isn't a bug, but could off + cnt - 1 be hoisted into a local here?
It appears eight times in this function now, and the two-line wrapped
assignment above exists only because the index expression is long.

>  	}
>
> +	/*
> +	 * Last slot instruction could be a newly generated
> +	 * BPF_ST/BPF_LDX/BPF_STX, systematically mark it for non-stack access
> +	 * if it is not the original instruction, otherwise keep the
> +	 * original marking
> +	 */
> +	if (bpf_is_mem_insn(insn + off + cnt - 1) &&
> +	    memcmp(insn + off + cnt - 1, original_insn, sizeof(struct bpf_insn)))
> +		data[off + cnt - 1].non_stack_access = true;

This isn't a bug, but does this comment add information beyond the if
condition below it? The comment reads as a prose transcription of the
conditional.


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