Re: [PATCH bpf-next v2 1/5] bpf/verifier: refactor checks for range computation

[Cupertino Miranda <cupertino.miranda@oracle.com>]

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.

>
> [...]