Re: [PATCH bpf-next v3 2/7] bpf: extract generic helper from process_timer_func()

[Eduard Zingerman <eddyz87@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1005 bytes --]

On Fri, 2025-09-05 at 17:45 +0100, Mykyta Yatsenko wrote:
> From: Mykyta Yatsenko <yatsenko@meta.com>
> 
> Refactor the verifier by pulling the common logic from
> process_timer_func() into a dedicated helper. This allows reusing
> process_async_func() helper for verifying bpf_task_work struct in the
> next patch.
> 
> Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com>
> ---

The discrepancy between bpf_call_arg_meta and bpf_kfunc_call_arg_meta
is unfortunate. Maybe the bigger refactoring is possible.
Wdyt about avoiding pointers to pointers and accepting some code
duplication for now? E.g. rename process_async_func:

  /* Check if @regno is a pointer to a specific field in a map value */
  static int check_map_field_pointer(struct bpf_verifier_env *env,
                                     u32 regno,
                                     enum btf_field_type field_type,
                                     u32 field_off)

And proceed as in the attached patch?

[...]

[-- Attachment #2: check_map_field_pointer.diff --]
[-- Type: text/x-patch, Size: 3254 bytes --]

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 6152536a834f..76dad7e0db5f 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -8522,15 +8522,17 @@ static int process_spin_lock(struct bpf_verifier_env *env, int regno, int flags)
 	return 0;
 }
 
-static int process_async_func(struct bpf_verifier_env *env, int regno, struct bpf_map **map_ptr,
-			      int *map_uid, u32 rec_off, enum btf_field_type field_type,
-			      const char *struct_name)
+/* Check if @regno is a pointer to a specific field in a map value */
+static int check_map_field_pointer(struct bpf_verifier_env *env,
+				   u32 regno,
+				   enum btf_field_type field_type,
+				   u32 field_off)
 {
 	struct bpf_reg_state *regs = cur_regs(env), *reg = &regs[regno];
+	const char *struct_name = btf_field_type_name(field_type);
 	bool is_const = tnum_is_const(reg->var_off);
 	struct bpf_map *map = reg->map_ptr;
 	u64 val = reg->var_off.value;
-	int *struct_off = (void *)map->record + rec_off;
 
 	if (!is_const) {
 		verbose(env,
@@ -8547,25 +8549,30 @@ static int process_async_func(struct bpf_verifier_env *env, int regno, struct bp
 		verbose(env, "map '%s' has no valid %s\n", map->name, struct_name);
 		return -EINVAL;
 	}
-	if (*struct_off != val + reg->off) {
+	if (field_off != val + reg->off) {
 		verbose(env, "off %lld doesn't point to 'struct %s' that is at %d\n",
-			val + reg->off, struct_name, *struct_off);
+			val + reg->off, struct_name, field_off);
 		return -EINVAL;
 	}
-	if (*map_ptr) {
-		verifier_bug(env, "Two map pointers in a %s helper", struct_name);
-		return -EFAULT;
-	}
-	*map_uid = reg->map_uid;
-	*map_ptr = map;
 	return 0;
 }
 
 static int process_timer_func(struct bpf_verifier_env *env, int regno,
 			      struct bpf_call_arg_meta *meta)
 {
-	return process_async_func(env, regno, &meta->map_ptr, &meta->map_uid,
-				  offsetof(struct btf_record, timer_off), BPF_TIMER, "bpf_timer");
+	struct bpf_reg_state *regs = cur_regs(env), *reg = &regs[regno];
+	int err;
+
+	err = check_map_field_pointer(env, regno, BPF_TIMER, offsetof(struct btf_record, timer_off));
+	if (err)
+		return err;
+
+	if (verifier_bug_if(meta->map_ptr, env, "Two map pointers in a bpf_timer helper"))
+		return -EFAULT;
+
+	meta->map_ptr = reg->map_ptr;
+	meta->map_uid = reg->map_uid;
+	return 0;
 }
 
 static int process_wq_func(struct bpf_verifier_env *env, int regno,
@@ -8588,9 +8595,20 @@ static int process_wq_func(struct bpf_verifier_env *env, int regno,
 static int process_task_work_func(struct bpf_verifier_env *env, int regno,
 				  struct bpf_kfunc_call_arg_meta *meta)
 {
-	return process_async_func(env, regno, &meta->map.ptr, &meta->map.uid,
-				  offsetof(struct btf_record, task_work_off), BPF_TASK_WORK,
-				  "bpf_task_work");
+	struct bpf_reg_state *regs = cur_regs(env), *reg = &regs[regno];
+	int err;
+
+	err = check_map_field_pointer(env, regno, BPF_TASK_WORK,
+				      offsetof(struct btf_record, task_work_off));
+	if (err)
+		return err;
+
+	if (verifier_bug_if(meta->map.ptr, env, "Two map pointers in a bpf_task_work helper"))
+		return -EFAULT;
+
+	meta->map.ptr = reg->map_ptr;
+	meta->map.uid = reg->map_uid;
+	return 0;
 }
 
 static int process_kptr_func(struct bpf_verifier_env *env, int regno,