Eduard Zingerman writes:
> On Wed, 2024-04-17 at 13:23 +0100, Cupertino Miranda wrote:
> [...]
>
>> @@ -13406,53 +13490,19 @@ static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env,
>
> [...]
>
>> - if (!src_known &&
>> - opcode != BPF_ADD && opcode != BPF_SUB && opcode != BPF_AND) {
>> + int is_safe = is_safe_to_compute_dst_reg_range(insn, src_reg);
>> + switch (is_safe) {
>> + case UNCOMPUTABLE_RANGE:
>> __mark_reg_unknown(env, dst_reg);
>> return 0;
>> + case UNDEFINED_BEHAVIOUR:
>> + mark_reg_unknown(env, regs, insn->dst_reg);
>> + return 0;
>> + default:
>> + break;
>> }
>
> Nit: I know that the division between __mark_reg_unknown() and
> mark_reg_unknown() was asked for directly, but tbh I don't think that
> it adds any value here, here is how mark_reg_unknown() is implemented:
>
> static void mark_reg_unknown(struct bpf_verifier_env *env,
> struct bpf_reg_state *regs, u32 regno)
> {
> if (WARN_ON(regno >= MAX_BPF_REG)) {
> ... mark all regs not init ...
> return;
> }
> __mark_reg_unknown(env, regs + regno);
> }
>
> The 'regno >= MAX_BPF_REG' does not apply here, because
> adjust_scalar_min_max_vals() is only called from the following stack:
> - check_alu_op
> - adjust_reg_min_max_vals
> - adjust_scalar_min_max_vals
>
> The check_alu_op() does check_reg_arg() which verifies that both src
> and dst register numbers are within bounds.
>
> I suggest to replace the enum with as boolean value.
> Miranda, Yonhong, what do you think?
Thanks for the detailed review.
Well, honestly I could not evaluate if there was any actual difference
between the approaches. Although I can understand range computation in
isolation of an instruction I still did not explore the code in the
global perspective, for example the handling of control-flow.
I was proud of the initial boolean implementation that was very clean
and simple, although like Yonghong said, not truly a refactor.
If everyone agrees that it is Ok, I will be happy to change it back.
>
> [...]
