Re: [PATCH bpf-next 4/5] bpf: simplify the bpf_is_reg64() signature

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] A comment explicitly referring to removed arguments of bpf_is_reg64() was left intact.
- [Low] The handling of BPF_JMP, BPF_JMP32, and BPF_ST classes inside bpf_is_reg64() is now completely dead code and contains misleading comments.
--

--- Patch [4]: [PATCH bpf-next 4/5] bpf: simplify the bpf_is_reg64() signature ---
commit ed53634b5ab05a53b4f5cd2fc3aac98c66a8ccd4
Author: Eduard Zingerman <[email protected]>

bpf: simplify the bpf_is_reg64() signature

After the previous commit this function is only used in a context
where destination register's property is queried.
Hence, simplify the function by removing the 'regno' and 't'
parameters.

> diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
> index 6125598d16a84..73ab95573f609 100644
> --- a/kernel/bpf/fixups.c
> +++ b/kernel/bpf/fixups.c

[ ... ]

> @@ -623,7 +623,7 @@ int bpf_opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env,
>  			 *       BPF_STX + SRC_OP, so it is safe to pass NULL
>  			 *       here.
>  			 */
> -			if (bpf_is_reg64(&insn, load_reg, NULL, DST_OP)) {
> +			if (bpf_is_reg64(&insn)) {

[Severity: Low]
Since the parameters have been removed from bpf_is_reg64(), should we also
remove or update this comment referring to the "fourth one"?

>  				if (class == BPF_LD &&
>  				    BPF_MODE(code) == BPF_IMM)
>  					i++;
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 0709dded013ea..d5b6fef1ad897 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -3028,11 +3028,10 @@ static void mark_stack_slots_scratched(struct bpf_verifier_env *env,
>  }
>  
>  /* This function is supposed to be used by the following 32-bit optimization
> - * code only. It returns TRUE if the source or destination register operates
> - * on 64-bit, otherwise return FALSE.
> + * code only. It returns TRUE if the destination register operates on 64-bit,
> + * otherwise return FALSE.
>   */
> -bool bpf_is_reg64(struct bpf_insn *insn,
> -	      u32 regno, struct bpf_reg_state *reg, enum bpf_reg_arg_type t)
> +bool bpf_is_reg64(struct bpf_insn *insn)

[Severity: Low]
Are the branches handling BPF_JMP, BPF_JMP32, and BPF_ST inside this function
now dead code?

The callers in bpf_insn_def32() and bpf_opt_subreg_zext_lo32_rnd_hi32() only 
evaluate bpf_is_reg64() if insn_def_regno() returns a value >= 0. 

Since insn_def_regno() returns -1 for BPF_JMP, BPF_JMP32, and BPF_ST, 
execution should never reach those branches here anymore. This leaves behind
unreachable code and makes the corresponding comments (like "BPF_EXIT for
'main' will reach here") inaccurate.

>  {
>  	u8 code, class, op;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4