Re: [PATCH bpf-next v4 08/12] bpf: Unify release handling for helpers and kfuncs

[Eduard Zingerman <eddyz87@gmail.com>]

On Wed, 2026-05-06 at 07:27 -0700, Amery Hung wrote:

LGTM, one question below.

> @@ -8010,28 +8010,15 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
>  		return err;
>  
>  skip_type_check:
> -	if (arg_type_is_release(arg_type)) {
> -		if (!arg_type_is_dynptr(arg_type) && !reg->ref_obj_id && !bpf_register_is_null(reg)) {
> -			verbose(env, "R%d must be referenced when passed to release function\n",
> -				regno);
> -			return -EINVAL;
> -		}
> -		if (meta->release_regno) {
> -			verifier_bug(env, "more than one release argument");
> -			return -EFAULT;
> -		}
> -		meta->release_regno = regno;
> +	if (arg_type_is_release(arg_type) && !arg_type_is_dynptr(arg_type) &&
> +	    !reg->ref_obj_id && !bpf_register_is_null(reg)) {
> +		verbose(env, "release helper %s expects referenced PTR_TO_BTF_ID passed to %s\n",
> +			func_id_name(meta->func_id), reg_arg_name(env, argno));
> +		return -EINVAL;

Nit: a similar check is added in check_kfunc_args(),
     can these be wrapped in some utility function?

>  	}
>  
> -	if (reg->ref_obj_id && base_type(arg_type) != ARG_KPTR_XCHG_DEST) {
> -		if (meta->release_regno && meta->ref_obj.cnt) {
> -			verbose(env, "more than one arg with ref_obj_id %s %u %u",
> -				reg_arg_name(env, argno), reg->ref_obj_id,
> -				meta->ref_obj.ref_obj_id);
> -			return -EACCES;
> -		}
> +	if (reg->ref_obj_id)
>  		update_ref_obj(&meta->ref_obj, reg);
> -	}
>  
>  	switch (base_type(arg_type)) {
>  	case ARG_CONST_MAP_PTR:

[...]

> @@ -9825,7 +9883,7 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
>  	memset(&meta, 0, sizeof(meta));
>  	meta.pkt_access = fn->pkt_access;
>  
> -	err = check_func_proto(fn);
> +	err = check_func_proto(fn, &meta);
>  	if (err) {
>  		verifier_bug(env, "incorrect func proto %s#%d", func_id_name(func_id), func_id);
>  		return err;
> @@ -9870,37 +9928,11 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
>  	}
>  
>  	if (meta.release_regno) {
> -		err = -EINVAL;
> -		if (arg_type_is_dynptr(fn->arg_type[meta.release_regno - BPF_REG_1])) {
> -			err = unmark_stack_slots_dynptr(env, &regs[meta.release_regno]);
> -		} else if (func_id == BPF_FUNC_kptr_xchg && meta.ref_obj.ref_obj_id) {
> -			u32 ref_obj_id = meta.ref_obj.ref_obj_id;
> -			bool in_rcu = in_rcu_cs(env);
> -			struct bpf_func_state *state;
> -			struct bpf_reg_state *reg;
> -
> -			err = release_reference_nomark(env->cur_state, ref_obj_id);
> -			if (!err) {
> -				bpf_for_each_reg_in_vstate(env->cur_state, state, reg, ({
> -					if (reg->ref_obj_id == ref_obj_id) {
> -						if (in_rcu && (reg->type & MEM_ALLOC) && (reg->type & MEM_PERCPU)) {
> -							reg->ref_obj_id = 0;
> -							reg->type &= ~MEM_ALLOC;
> -							reg->type |= MEM_RCU;
> -						} else {
> -							mark_reg_invalid(env, reg);

Note: release_reg() does not have this 'else' path, but it seem to be not a problem,
      assuming that MEM_ALLOC | MEM_PERCPU registers are generated only from some
      other cmp_xchg call. Is that the case?

> -						}
> -					}
> -				}));
> -			}
> -		} else if (meta.ref_obj.ref_obj_id) {
> -			err = release_reference(env, meta.ref_obj.ref_obj_id);
> -		} else if (bpf_register_is_null(&regs[meta.release_regno])) {
> -			/* meta.ref_obj.ref_obj_id can only be 0 if register that is meant to be
> -			 * released is NULL, which must be > R0.
> -			 */
> -			err = 0;
> -		}
> +		struct bpf_reg_state *reg = &regs[meta.release_regno];
> +		bool convert_rcu = (func_id == BPF_FUNC_kptr_xchg) && in_rcu_cs(env) &&
> +				   (reg->type & MEM_ALLOC) && (reg->type & MEM_PERCPU);
> +
> +		err = release_reg(env, reg, convert_rcu, !!meta.dynptr.ref_obj_id);
>  		if (err)
>  			return err;
>  	}

[...]

> @@ -11609,18 +11640,16 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
>  			return -EACCES;
>  		}
>  
> -		if (reg->ref_obj_id) {
> -			if (is_kfunc_release(meta) && meta->ref_obj.cnt) {
> -				verbose(env, "more than one arg with ref_obj_id %s %u %u",
> -					reg_arg_name(env, argno), reg->ref_obj_id,
> -					meta->ref_obj.ref_obj_id);
> -				return -EFAULT;
> -			}
> -			update_ref_obj(&meta->ref_obj, reg);
> -			if (is_kfunc_release(meta))
> -				meta->release_regno = regno;
> +		if (regno == meta->release_regno && !is_kfunc_arg_dynptr(meta->btf, &args[i]) &&
> +		    !reg->ref_obj_id && !bpf_register_is_null(reg)) {
> +			verbose(env, "release kfunc %s expects referenced PTR_TO_BTF_ID passed to %s\n",
> +				func_name, reg_arg_name(env, argno));
> +			return -EINVAL;
>  		}
>  
> +		if (reg->ref_obj_id)
> +			update_ref_obj(&meta->ref_obj, reg);
> +
>  		ref_t = btf_type_skip_modifiers(btf, t->type, &ref_id);
>  		ref_tname = btf_name_by_offset(btf, ref_t->name_off);
>  

[...]