Re: [PATCH bpf-next v3 1/2] bpf: Do not include stack ptr register in precision backtracking bookkeeping

[Eduard Zingerman <eddyz87@gmail.com>]

[...]

> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index 78c97e12ea4e..e73a910e4ece 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -357,6 +357,10 @@ enum {
>   	INSN_F_SPI_SHIFT = 3, /* shifted 3 bits to the left */
>   
>   	INSN_F_STACK_ACCESS = BIT(9), /* we need 10 bits total */
> +
> +	INSN_F_DST_REG_STACK = BIT(10), /* dst_reg is PTR_TO_STACK */
> +	INSN_F_SRC_REG_STACK = BIT(11), /* src_reg is PTR_TO_STACK */

INSN_F_STACK_ACCESS can be inferred from INSN_F_DST_REG_STACK
and INSN_F_SRC_REG_STACK if insn_stack_access_flags() is adjusted
to track these flags instead. So, can be one less flag/bit.

> +	/* total 12 bits are used now. */
>   };
>   
>   static_assert(INSN_F_FRAMENO_MASK + 1 >= MAX_CALL_FRAMES);

[...]

> @@ -4402,6 +4418,8 @@ static int backtrack_insn(struct bpf_verifier_env *env, int idx, int subseq_idx,
>   			 */
>   			return 0;
>   		} else if (BPF_SRC(insn->code) == BPF_X) {
> +			bool dreg_precise, sreg_precise;
> +
>   			if (!bt_is_reg_set(bt, dreg) && !bt_is_reg_set(bt, sreg))
>   				return 0;
>   			/* dreg <cond> sreg
> @@ -4410,8 +4428,16 @@ static int backtrack_insn(struct bpf_verifier_env *env, int idx, int subseq_idx,
>   			 * before it would be equally necessary to
>   			 * propagate it to dreg.
>   			 */
> -			bt_set_reg(bt, dreg);
> -			bt_set_reg(bt, sreg);
> +			if (!hist)
> +				return 0;
> +			dreg_precise = !insn_dreg_stack_ptr(hist->flags);
> +			sreg_precise = !insn_sreg_stack_ptr(hist->flags);
> +			if (!dreg_precise && !sreg_precise)
> +				return 0;
> +			if (dreg_precise)
> +				bt_set_reg(bt, dreg);
> +			if (sreg_precise)
> +				bt_set_reg(bt, sreg);

This check can be done in a generic way at backtrack_insn() callsite:
check which register is pointer to stack and remove it from set registers.

>   		} else if (BPF_SRC(insn->code) == BPF_K) {
>   			 /* dreg <cond> K
>   			  * Only dreg still needs precision before
> @@ -16397,6 +16423,29 @@ static void sync_linked_regs(struct bpf_verifier_state *vstate, struct bpf_reg_s
>   	}
>   }
>   
> +static int push_cond_jmp_history(struct bpf_verifier_env *env, struct bpf_verifier_state *state,
> +				 struct bpf_reg_state *dst_reg, struct bpf_reg_state *src_reg,
> +				 u64 linked_regs)
> +{
> +	bool dreg_stack_ptr, sreg_stack_ptr;
> +	int insn_flags;
> +
> +	if (!src_reg) {
> +		if (linked_regs)
> +			return push_insn_history(env, state, 0, linked_regs);
> +		return 0;
> +	}

Nit: this 'if' is not needed, src_reg is always set (it might point to a 
fake register,
      but in that case it is a scalar without id).

> +
> +	dreg_stack_ptr = dst_reg->type == PTR_TO_STACK;
> +	sreg_stack_ptr = src_reg->type == PTR_TO_STACK;
> +
> +	if (!dreg_stack_ptr && !sreg_stack_ptr && !linked_regs)
> +		return 0;
> +
> +	insn_flags = insn_reg_access_flags(dreg_stack_ptr, sreg_stack_ptr);
> +	return push_insn_history(env, state, insn_flags, linked_regs);
> +}
> +

[...]