Re: [PATCH 6.12 541/602] bpf: Refactor {acquire,release}_reference_state

Hi Greg/Sasha,


On 30/07/26 7:45 pm, Greg Kroah-Hartman wrote:
> 6.12-stable review patch.  If anyone has any objections, please let me know.
> 
> ------------------
> 
> From: Kumar Kartikeya Dwivedi <[email protected]>
> 
> [ Upstream commit 769b0f1c821455ab29baf42491e1ea1d726451fa ]
> 

^^ note this.

> In preparation for introducing support for more reference types which
> have to add and remove reference state, refactor the
> acquire_reference_state and release_reference_state functions to share
> common logic.
> 
> The acquire_reference_state function simply handles growing the acquired
> refs and returning the pointer to the new uninitialized element, which
> can be filled in by the caller.
> 
> The release_reference_state function simply erases a reference state
> entry in the acquired_refs array and shrinks it. The callers are
> responsible for finding the suitable element by matching on various
> fields of the reference state and requesting deletion through this
> function. It is not supposed to be called directly.
> 
> Existing callers of release_reference_state were using it to find and
> remove state for a given ref_obj_id without scrubbing the associated
> registers in the verifier state. Introduce release_reference_nomark to
> provide this functionality and convert callers. We now use this new
> release_reference_nomark function within release_reference as well.
> It needs to operate on a verifier state instead of taking verifier env
> as mark_ptr_or_null_regs requires operating on verifier state of the
> two branches of a NULL condition check, therefore env->cur_state cannot
> be used directly.
> 

None of that happens in the backport actually.

vimdiff <(git show 76344b7301ce) <(git show 769b0f1c821455) shows a 
bunch of differences.




> Acked-by: Eduard Zingerman <[email protected]>
> Signed-off-by: Kumar Kartikeya Dwivedi <[email protected]>
> Link: https://lore.kernel.org/r/[email protected]
> Signed-off-by: Alexei Starovoitov <[email protected]>
> Stable-dep-of: 5e0b273e0a62 ("bpf: Reset register bounds before narrowing retval range in check_mem_access()")
> Signed-off-by: Sasha Levin <[email protected]>
> Signed-off-by: Greg Kroah-Hartman <[email protected]>
> ---
>   kernel/bpf/verifier.c |   28 +++++++++++++++-------------
>   1 file changed, 15 insertions(+), 13 deletions(-)
> 
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -7207,11 +7207,12 @@ static int check_mem_access(struct bpf_v
>   		if (!err && value_regno >= 0 && (t == BPF_READ || rdonly_mem))
>   			mark_reg_unknown(env, regs, value_regno);
>   	} else if (reg->type == PTR_TO_CTX) {
> -		bool is_retval = false;
> +		struct bpf_insn_access_aux info = {
> +			.reg_type = SCALAR_VALUE,
> +			.is_ldsx = is_ldsx,
> +			.log = &env->log,
> +		};
>   		struct bpf_retval_range range;
> -		enum bpf_reg_type reg_type = SCALAR_VALUE;
> -		struct btf *btf = NULL;
> -		u32 btf_id = 0;

^^ this is actually done in commit: 201b62ccc831 ("bpf: Refactor 
check_ctx_access()") which is next commit.

I have run an AI assisted backport review and it spotted a commit 
mapping problem in the BPF prerequisite series.

76344b7301ce claims upstream 769b0f1c8214 ("bpf: Refactor 
{acquire,release}_reference_state"). Upstream adds acquire_reference(), 
release_reference_nomark(), and changes acquire_reference_state() to 
return a reference-state pointer.

The 6.12.y commit leaves the old int-returning acquire_reference_state() 
in place and has none of those new helpers.
Its diff is instead the first part of moving check_mem_access()
to struct bpf_insn_access_aux, which belongs to upstream
201b62ccc831 ("bpf: Refactor check_ctx_access()").

The next stable patch commit 4ee2d0ce3e82 claims 201b62ccc831 and 
completes that split before (patch 543) 6261a33d665b applies the actual 
bounds fix.

So 769b0f1c8214 is recorded as present although its semantics are
absent, while 201b62ccc831 is split across two differently attributed
commits.

Maybe we could be replacing 76344b7301ce plus 4ee2d0ce3e82 with one
correctly attributed adapted backport of 201b62ccc831 before
6261a33d665b.

Thoughts ?

thanks,
Harshit


>   
>   		if (t == BPF_WRITE && value_regno >= 0 &&
>   		    is_pointer_value(env, value_regno)) {
> @@ -7223,8 +7224,9 @@ static int check_mem_access(struct bpf_v
>   		if (err < 0)
>   			return err;
>   
> -		err = check_ctx_access(env, insn_idx, off, size, t, &reg_type, &btf,
> -				       &btf_id, &is_retval, is_ldsx);
> +		err = check_ctx_access(env, insn_idx, off, size, t, &info.reg_type,
> +				       &info.btf, &info.btf_id, &info.is_retval,
> +				       info.is_ldsx);
>   		if (err)
>   			verbose_linfo(env, insn_idx, "; ");
>   		if (!err && t == BPF_READ && value_regno >= 0) {
> @@ -7232,8 +7234,8 @@ static int check_mem_access(struct bpf_v
>   			 * PTR_TO_PACKET[_META,_END]. In the latter
>   			 * case, we know the offset is zero.
>   			 */
> -			if (reg_type == SCALAR_VALUE) {
> -				if (is_retval && get_func_retval_range(env->prog, &range)) {
> +			if (info.reg_type == SCALAR_VALUE) {
> +				if (info.is_retval && get_func_retval_range(env->prog, &range)) {
>   					err = __mark_reg_s32_range(env, regs, value_regno,
>   								   range.minval, range.maxval);
>   					if (err)
> @@ -7244,7 +7246,7 @@ static int check_mem_access(struct bpf_v
>   			} else {
>   				mark_reg_known_zero(env, regs,
>   						    value_regno);
> -				if (type_may_be_null(reg_type))
> +				if (type_may_be_null(info.reg_type))
>   					regs[value_regno].id = ++env->id_gen;
>   				/* A load of ctx field could have different
>   				 * actual load size with the one encoded in the
> @@ -7252,12 +7254,12 @@ static int check_mem_access(struct bpf_v
>   				 * a sub-register.
>   				 */
>   				regs[value_regno].subreg_def = DEF_NOT_SUBREG;
> -				if (base_type(reg_type) == PTR_TO_BTF_ID) {
> -					regs[value_regno].btf = btf;
> -					regs[value_regno].btf_id = btf_id;
> +				if (base_type(info.reg_type) == PTR_TO_BTF_ID) {
> +					regs[value_regno].btf = info.btf;
> +					regs[value_regno].btf_id = info.btf_id;
>   				}
>   			}
> -			regs[value_regno].type = reg_type;
> +			regs[value_regno].type = info.reg_type;
>   		}
>   
>   	} else if (reg->type == PTR_TO_STACK) {
> 
> 
>