Re: [PATCH bpf-next v2 1/4] bpf: bpf task work plumbing

[Eduard Zingerman <eddyz87@gmail.com>]

On Fri, 2025-08-15 at 20:21 +0100, Mykyta Yatsenko wrote:
> From: Mykyta Yatsenko <yatsenko@meta.com>
> 
> This patch adds necessary plumbing in verifier, syscall and maps to
> support handling new kfunc bpf_task_work_schedule and kernel structure
> bpf_task_work. The idea is similar to how we already handle bpf_wq and
> bpf_timer.
> verifier changes validate calls to bpf_task_work_schedule to make sure
> it is safe and expected invariants hold.
> btf part is required to detect bpf_task_work structure inside map value
> and store its offset, which will be used in the next patch to calculate
> key and value addresses.
> arraymap and hashtab changes are needed to handle freeing of the
> bpf_task_work: run code needed to deinitialize it, for example cancel
> task_work callback if possible.
> The use of bpf_task_work and proper implementation for kfuncs are
> introduced in the next patch.
> 
> Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com>
> ---

Amount of copy-paste necessary for dealing with objects btf is saddening.
This patch follows current approach and seem to do it correctly.

[...]

> --- a/kernel/bpf/arraymap.c
> +++ b/kernel/bpf/arraymap.c
> @@ -431,7 +431,7 @@ static void *array_map_vmalloc_addr(struct bpf_array *array)
>  	return (void *)round_down((unsigned long)array, PAGE_SIZE);
>  }
>  
> -static void array_map_free_timers_wq(struct bpf_map *map)
> +static void array_map_free_internal_structs(struct bpf_map *map)
>  {
>  	struct bpf_array *array = container_of(map, struct bpf_array, map);
>  	int i;
> @@ -439,12 +439,14 @@ static void array_map_free_timers_wq(struct bpf_map *map)
>  	/* We don't reset or free fields other than timer and workqueue
>  	 * on uref dropping to zero.
>  	 */
> -	if (btf_record_has_field(map->record, BPF_TIMER | BPF_WORKQUEUE)) {
> +	if (btf_record_has_field(map->record, BPF_TIMER | BPF_WORKQUEUE | BPF_TASK_WORK)) {

Is there a way to share this code between array map and hash map?

>  		for (i = 0; i < array->map.max_entries; i++) {
>  			if (btf_record_has_field(map->record, BPF_TIMER))
>  				bpf_obj_free_timer(map->record, array_map_elem_ptr(array, i));
>  			if (btf_record_has_field(map->record, BPF_WORKQUEUE))
>  				bpf_obj_free_workqueue(map->record, array_map_elem_ptr(array, i));
> +			if (btf_record_has_field(map->record, BPF_TASK_WORK))
> +				bpf_obj_free_task_work(map->record, array_map_elem_ptr(array, i));
>  		}
>  	}
>  }

[...]

> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 64739308902f..378f260235dd 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -3527,6 +3527,15 @@ static int btf_get_field_type(const struct btf *btf, const struct btf_type *var_
>  			goto end;
>  		}
>  	}
> +	if (field_mask & BPF_TASK_WORK) {
> +		if (!strcmp(name, "bpf_task_work")) {
> +			if (*seen_mask & BPF_TASK_WORK)
> +				return -E2BIG;
> +			*seen_mask |= BPF_TASK_WORK;
> +			type = BPF_TASK_WORK;
> +			goto end;
> +		}
> +	}

Nit: extract this and ifs before it as a loop over array
     of name/flag pairs?

>  	field_mask_test_name(BPF_LIST_HEAD, "bpf_list_head");
>  	field_mask_test_name(BPF_LIST_NODE, "bpf_list_node");
>  	field_mask_test_name(BPF_RB_ROOT,   "bpf_rb_root");

[...]

> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> index 71f9931ac64c..207ad4823b5b 100644
> --- a/kernel/bpf/hashtab.c
> +++ b/kernel/bpf/hashtab.c

[...]

> @@ -233,6 +233,9 @@ static void htab_free_prealloced_timers_and_wq(struct bpf_htab *htab)
>  		if (btf_record_has_field(htab->map.record, BPF_WORKQUEUE))
>  			bpf_obj_free_workqueue(htab->map.record,
>  					       htab_elem_value(elem, htab->map.key_size));
> +		if (btf_record_has_field(htab->map.record, BPF_TASK_WORK))
> +			bpf_obj_free_task_work(htab->map.record,
> +					       htab_elem_value(elem, htab->map.key_size));

If there is no generic way to share this code with array maps,
please, at-least within the hashmap.c extract these "if (btf_record_has_field(...)) {...}"
groups so that there is no duplication between
htab_free_{malloced,preallocated}_internal_structs(htab).

>  		cond_resched();
>  	}
>  }

[...]

> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 0fbfa8532c39..108d86f7eeaf 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c

[...]

> @@ -1309,6 +1322,14 @@ static int map_check_btf(struct bpf_map *map, struct bpf_token *token,
>  					goto free_map_tab;
>  				}
>  				break;
> +			case BPF_TASK_WORK:

This can be added to the group with BPF_TIMER and BPF_WORKQUEUE just above.

> +				if (map->map_type != BPF_MAP_TYPE_HASH &&
> +				    map->map_type != BPF_MAP_TYPE_LRU_HASH &&
> +				    map->map_type != BPF_MAP_TYPE_ARRAY) {
> +					ret = -EOPNOTSUPP;
> +					goto free_map_tab;
> +				}
> +				break;
>  			default:
>  				/* Fail if map_type checks are missing for a field type */
>  				ret = -EOPNOTSUPP;
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index a61d57996692..be7a744c7917 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[...]

This function repeats process_timer_func() almost verbatim.

> +{
> +	struct bpf_reg_state *regs = cur_regs(env), *reg = &regs[regno];
> +	struct bpf_map *map = reg->map_ptr;
> +	bool is_const = tnum_is_const(reg->var_off);
> +	u64 val = reg->var_off.value;
> +
> +	if (!map->btf) {
> +		verbose(env, "map '%s' has to have BTF in order to use bpf_task_work\n",
> +			map->name);
> +		return -EINVAL;
> +	}
> +	if (!btf_record_has_field(map->record, BPF_TASK_WORK)) {
> +		verbose(env, "map '%s' has no valid bpf_task_work\n", map->name);
> +		return -EINVAL;
> +	}
> +	if (!is_const) {
> +		verbose(env,
> +			"bpf_task_work has to be at the constant offset\n");
> +		return -EINVAL;
> +	}
> +	if (map->record->task_work_off != val + reg->off) {
> +		verbose(env,
> +			"off %lld doesn't point to 'struct bpf_task_work' that is at %d\n",
> +			val + reg->off, map->record->task_work_off);
> +		return -EINVAL;
> +	}
> +	if (meta->map.ptr) {
> +		verifier_bug(env, "Two map pointers in a bpf_task_work kfunc");
> +		return -EFAULT;
> +	}
> +
> +	meta->map.uid = reg->map_uid;
> +	meta->map.ptr = map;
> +	return 0;
> +}
> +
>  static int process_kptr_func(struct bpf_verifier_env *env, int regno,
>  			     struct bpf_call_arg_meta *meta)
>  {

[...]