Re: [PATCH bpf v2 6/7] bpf: Assign lock identity to callback map values

[Eduard Zingerman <eddyz87@gmail.com>]

On Sat, 2026-09-05 at 10:34 +0200, Kumar Kartikeya Dwivedi wrote:
> The verifier identifies the allocation containing a bpf_spin_lock by the
> pair of the map pointer and register ID. It permits ID zero for direct
> map-value loads into single-element array maps because they have one stable
> value.
> 
> Callback frame constructors also leave map-value arguments with ID zero,
> but their maps can have multiple elements. Consequently, nested callbacks
> can hold two distinct elements of the same map with an identical lock
> identity. The verifier then permits a lock acquired through one element to
> be released through another. The same confusion lets graph kfuncs operate
> on one element while another element is locked, allowing concurrent list
> corruption.
> 
> Give lockable callback map values a fresh ID in the for-each,
> timer/workqueue, and task-work frame constructors. Keep ID zero for
> top-level one-element array maps, whose callback argument and pseudo
> map-value load alias the same stable allocation. Maps without locks remain
> unchanged, while copies of one callback value continue to share an ID and
> support balanced locking.
> 
> Map-in-map lookups need additional care. Distinct concrete inner maps share
> the verifier-visible inner_map_meta, so a one-element inner array otherwise
> looks like the same static allocation. Preserve lookup identity as map_uid
> for lock-bearing inner maps and consult it before applying the ID-zero
> exception. Restricting UID propagation to maps with identity-sensitive
> fields avoids making state pruning conservative for every inner map.

I find the above commit message extremely hard to parse.
Please replace it with a small repro program and an explanation of
what fails in code.

> Fixes: d0d78c1df9b1 ("bpf: Allow locking bpf_spin_lock global variables")
> Reported-by: Nicholas Carlini <npc@anthropic.com>
> Suggested-by: Nicholas Carlini <npc@anthropic.com>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---
>  kernel/bpf/verifier.c | 29 ++++++++++++++++++++++++++---
>  1 file changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 9c797cc3df40..ad14a7fbed72 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -448,6 +448,13 @@ static bool reg_may_point_to_spin_lock(const struct bpf_reg_state *reg)
>  	return btf_record_has_field(reg_btf_record(reg), BPF_SPIN_LOCK | BPF_RES_SPIN_LOCK);
>  }
>  
> +static bool map_value_has_static_identity(const struct bpf_reg_state *reg)
> +{
> +	const struct bpf_map *map = reg->map_ptr;
> +
> +	return !reg->map_uid && map->map_type == BPF_MAP_TYPE_ARRAY && map->max_entries == 1;
> +}
> +
>  static bool type_is_rdonly_mem(u32 type)
>  {
>  	return type & MEM_RDONLY;
> @@ -1926,11 +1933,18 @@ static void refine_map_lookup_value(struct bpf_reg_state *reg)
>  	if (map->inner_map_meta) {
>  		reg->type = CONST_PTR_TO_MAP | maybe_null;
>  		reg->map_ptr = map->inner_map_meta;
> -		/* transfer reg's id which is unique for every map_lookup_elem
> -		 * as UID of the inner map.
> +		/*
> +		 * Concrete inner maps share the verifier-visible inner_map_meta.
> +		 * Preserve the lookup identity only for embedded objects whose
> +		 * verification needs to distinguish concrete map instances. Doing
> +		 * this for every inner map makes state pruning too conservative.
> +		 *
> +		 * Each map lookup has a unique register ID, so use it as the UID of
> +		 * the inner map.
>  		 */
>  		if (btf_record_has_field(map->inner_map_meta->record,
> -					 BPF_TIMER | BPF_WORKQUEUE | BPF_TASK_WORK))
> +					 BPF_TIMER | BPF_WORKQUEUE | BPF_TASK_WORK |
> +					 BPF_SPIN_LOCK | BPF_RES_SPIN_LOCK))

I'd drop the condition above altogether and route map_uid through
check_ids().

>  			reg->map_uid = reg->id;
>  	} else if (map->map_type == BPF_MAP_TYPE_XSKMAP) {
>  		reg->type = PTR_TO_XDP_SOCK | maybe_null;
> @@ -10036,6 +10050,9 @@ int map_set_for_each_callback_args(struct bpf_verifier_env *env,
>  	__mark_reg_known_zero(&callee->regs[BPF_REG_3]);
>  	callee->regs[BPF_REG_3].map_ptr = caller->regs[BPF_REG_1].map_ptr;
>  	callee->regs[BPF_REG_3].map_uid = caller->regs[BPF_REG_1].map_uid;
> +	if (reg_may_point_to_spin_lock(&callee->regs[BPF_REG_3]) &&
> +	    !map_value_has_static_identity(&callee->regs[BPF_REG_3]))

reg_may_point_to_spin_lock() check here and below is redundant. Let's
drop it and save ourselves from necessity to analyze in which cases
PTR_TO_MAP_VALUE requires an .id.

map_value_has_static_identity() -- I don't think this predicate is
necessary either. Can you construct a realistic program requiring such
special case?

> +		callee->regs[BPF_REG_3].id = ++env->id_gen;
>  
>  	/* pointer to stack or null */
>  	callee->regs[BPF_REG_4] = caller->regs[BPF_REG_3];
> @@ -10132,6 +10149,9 @@ static int set_timer_callback_state(struct bpf_verifier_env *env,
>  	__mark_reg_known_zero(&callee->regs[BPF_REG_3]);
>  	callee->regs[BPF_REG_3].map_ptr = map_ptr;
>  	callee->regs[BPF_REG_3].map_uid = map_uid;
> +	if (reg_may_point_to_spin_lock(&callee->regs[BPF_REG_3]) &&
> +	    !map_value_has_static_identity(&callee->regs[BPF_REG_3]))
> +		callee->regs[BPF_REG_3].id = ++env->id_gen;
>  
>  	/* unused */
>  	bpf_mark_reg_not_init(env, &callee->regs[BPF_REG_4]);
> @@ -10250,6 +10270,9 @@ static int set_task_work_schedule_callback_state(struct bpf_verifier_env *env,
>  	__mark_reg_known_zero(&callee->regs[BPF_REG_3]);
>  	callee->regs[BPF_REG_3].map_ptr = map_ptr;
>  	callee->regs[BPF_REG_3].map_uid = map_uid;
> +	if (reg_may_point_to_spin_lock(&callee->regs[BPF_REG_3]) &&
> +	    !map_value_has_static_identity(&callee->regs[BPF_REG_3]))
> +		callee->regs[BPF_REG_3].id = ++env->id_gen;
>  
>  	/* unused */
>  	bpf_mark_reg_not_init(env, &callee->regs[BPF_REG_4]);