[PATCH bpf-next] bpf: replace register_is_const() with is_reg_const()

[PATCH bpf-next] bpf: replace register_is_const() with is_reg_const()

[Shung-Hsi Yu]

The addition of is_reg_const() in commit 171de12646d2 ("bpf: generalize
is_branch_taken to handle all conditional jumps in one place") has made the
register_is_const() redundent. Give the former has more feature, plus the
fact the latter is only used in one place, replace register_is_const() with
is_reg_const(), and remove the definition of register_is_const.

This requires moving the definition of is_reg_const() further up. And since
the comment of reg_const_value() reference is_reg_const(), move it up as
well.

Signed-off-by: Shung-Hsi Yu <shung-hsi.yu@suse.com>
---
 kernel/bpf/verifier.c | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 2197385d91dc..a7651a861e42 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -4685,9 +4685,17 @@ static bool register_is_null(struct bpf_reg_state *reg)
 	return reg->type == SCALAR_VALUE && tnum_equals_const(reg->var_off, 0);
 }
 
-static bool register_is_const(struct bpf_reg_state *reg)
+/* check if register is a constant scalar value */
+static bool is_reg_const(struct bpf_reg_state *reg, bool subreg32)
 {
-	return reg->type == SCALAR_VALUE && tnum_is_const(reg->var_off);
+	return reg->type == SCALAR_VALUE &&
+	       tnum_is_const(subreg32 ? tnum_subreg(reg->var_off) : reg->var_off);
+}
+
+/* assuming is_reg_const() is true, return constant value of a register */
+static u64 reg_const_value(struct bpf_reg_state *reg, bool subreg32)
+{
+	return subreg32 ? tnum_subreg(reg->var_off).value : reg->var_off.value;
 }
 
 static bool __is_scalar_unbounded(struct bpf_reg_state *reg)
@@ -10030,7 +10038,7 @@ record_func_key(struct bpf_verifier_env *env, struct bpf_call_arg_meta *meta,
 	val = reg->var_off.value;
 	max = map->max_entries;
 
-	if (!(register_is_const(reg) && val < max)) {
+	if (!(is_reg_const(reg, false) && val < max)) {
 		bpf_map_key_store(aux, BPF_MAP_KEY_POISON);
 		return 0;
 	}
@@ -14167,19 +14175,6 @@ static void find_good_pkt_pointers(struct bpf_verifier_state *vstate,
 	}));
 }
 
-/* check if register is a constant scalar value */
-static bool is_reg_const(struct bpf_reg_state *reg, bool subreg32)
-{
-	return reg->type == SCALAR_VALUE &&
-	       tnum_is_const(subreg32 ? tnum_subreg(reg->var_off) : reg->var_off);
-}
-
-/* assuming is_reg_const() is true, return constant value of a register */
-static u64 reg_const_value(struct bpf_reg_state *reg, bool subreg32)
-{
-	return subreg32 ? tnum_subreg(reg->var_off).value : reg->var_off.value;
-}
-
 /*
  * <reg1> <op> <reg2>, currently assuming reg2 is a constant
  */

base-commit: 856624f12b04a3f51094fa277a31a333ee81cb3f
-- 
2.42.0

Re: [PATCH bpf-next] bpf: replace register_is_const() with is_reg_const()

[Andrii Nakryiko]

On Wed, Nov 8, 2023 at 6:01 AM Shung-Hsi Yu <shung-hsi.yu@suse.com> wrote:
>
> The addition of is_reg_const() in commit 171de12646d2 ("bpf: generalize
> is_branch_taken to handle all conditional jumps in one place") has made the
> register_is_const() redundent. Give the former has more feature, plus the

typo: redundant

> fact the latter is only used in one place, replace register_is_const() with
> is_reg_const(), and remove the definition of register_is_const.
>
> This requires moving the definition of is_reg_const() further up. And since
> the comment of reg_const_value() reference is_reg_const(), move it up as
> well.
>
> Signed-off-by: Shung-Hsi Yu <shung-hsi.yu@suse.com>
> ---
>  kernel/bpf/verifier.c | 27 +++++++++++----------------
>  1 file changed, 11 insertions(+), 16 deletions(-)
>

I didn't notice duplication, but agree that it's best to unify. Thanks

Acked-by: Andrii Nakryiko <andrii@kernel.org>

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 2197385d91dc..a7651a861e42 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -4685,9 +4685,17 @@ static bool register_is_null(struct bpf_reg_state *reg)
>         return reg->type == SCALAR_VALUE && tnum_equals_const(reg->var_off, 0);
>  }
>
> -static bool register_is_const(struct bpf_reg_state *reg)
> +/* check if register is a constant scalar value */
> +static bool is_reg_const(struct bpf_reg_state *reg, bool subreg32)
>  {
> -       return reg->type == SCALAR_VALUE && tnum_is_const(reg->var_off);
> +       return reg->type == SCALAR_VALUE &&
> +              tnum_is_const(subreg32 ? tnum_subreg(reg->var_off) : reg->var_off);
> +}
> +
> +/* assuming is_reg_const() is true, return constant value of a register */
> +static u64 reg_const_value(struct bpf_reg_state *reg, bool subreg32)
> +{
> +       return subreg32 ? tnum_subreg(reg->var_off).value : reg->var_off.value;
>  }
>
>  static bool __is_scalar_unbounded(struct bpf_reg_state *reg)
> @@ -10030,7 +10038,7 @@ record_func_key(struct bpf_verifier_env *env, struct bpf_call_arg_meta *meta,
>         val = reg->var_off.value;
>         max = map->max_entries;
>
> -       if (!(register_is_const(reg) && val < max)) {
> +       if (!(is_reg_const(reg, false) && val < max)) {
>                 bpf_map_key_store(aux, BPF_MAP_KEY_POISON);
>                 return 0;
>         }
> @@ -14167,19 +14175,6 @@ static void find_good_pkt_pointers(struct bpf_verifier_state *vstate,
>         }));
>  }
>
> -/* check if register is a constant scalar value */
> -static bool is_reg_const(struct bpf_reg_state *reg, bool subreg32)
> -{
> -       return reg->type == SCALAR_VALUE &&
> -              tnum_is_const(subreg32 ? tnum_subreg(reg->var_off) : reg->var_off);
> -}
> -
> -/* assuming is_reg_const() is true, return constant value of a register */
> -static u64 reg_const_value(struct bpf_reg_state *reg, bool subreg32)
> -{
> -       return subreg32 ? tnum_subreg(reg->var_off).value : reg->var_off.value;
> -}
> -
>  /*
>   * <reg1> <op> <reg2>, currently assuming reg2 is a constant
>   */
>
> base-commit: 856624f12b04a3f51094fa277a31a333ee81cb3f
> --
> 2.42.0
>

Re: [PATCH bpf-next] bpf: replace register_is_const() with is_reg_const()

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:

On Wed,  8 Nov 2023 22:00:41 +0800 you wrote:
> The addition of is_reg_const() in commit 171de12646d2 ("bpf: generalize
> is_branch_taken to handle all conditional jumps in one place") has made the
> register_is_const() redundent. Give the former has more feature, plus the
> fact the latter is only used in one place, replace register_is_const() with
> is_reg_const(), and remove the definition of register_is_const.
> 
> This requires moving the definition of is_reg_const() further up. And since
> the comment of reg_const_value() reference is_reg_const(), move it up as
> well.
> 
> [...]

Here is the summary with links:
  - [bpf-next] bpf: replace register_is_const() with is_reg_const()
    https://git.kernel.org/bpf/bpf-next/c/3815f89592d5

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html