Received-SPF: pass (google.com: domain of linux-kernel+bounces-151031-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Thu, 18 Apr 2024 23:00:58 -0700
From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: Benjamin Tissoires <bentiss@kernel.org>
Cc: Alexei Starovoitov <ast@kernel.org>, 
	Daniel Borkmann <daniel@iogearbox.net>, Andrii Nakryiko <andrii@kernel.org>, 
	Martin KaFai Lau <martin.lau@linux.dev>, Eduard Zingerman <eddyz87@gmail.com>, Song Liu <song@kernel.org>, 
	Yonghong Song <yonghong.song@linux.dev>, John Fastabend <john.fastabend@gmail.com>, 
	KP Singh <kpsingh@kernel.org>, Stanislav Fomichev <sdf@google.com>, Hao Luo <haoluo@google.com>, 
	Jiri Olsa <jolsa@kernel.org>, Mykola Lysenko <mykolal@fb.com>, Shuah Khan <shuah@kernel.org>, 
	bpf@vger.kernel.org, linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org
Subject: Re: [PATCH bpf-next 08/18] bpf: add support for
 KF_ARG_PTR_TO_WORKQUEUE
Message-ID: <wkcvx6cgti2mowyn646usogqe5ebafsj2k5oejn7tbvzgwttfb@c62ktm7za5ru>
References: <20240416-bpf_wq-v1-0-c9e66092f842@kernel.org>
 <20240416-bpf_wq-v1-8-c9e66092f842@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240416-bpf_wq-v1-8-c9e66092f842@kernel.org>

On Tue, Apr 16, 2024 at 04:08:21PM +0200, Benjamin Tissoires wrote:
> Introduce support for KF_ARG_PTR_TO_WORKQUEUE. The kfuncs will use bpf_wq
> as argument and that will be recognized as workqueue argument by verifier.
> bpf_wq_kern casting can happen inside kfunc, but using bpf_wq in
> argument makes life easier for users who work with non-kern type in BPF
> progs.
> 
> Duplicate process_timer_func into process_wq_func.
> meta argument is only needed to ensure bpf_wq_init's workqueue and map
> arguments are coming from the same map (map_uid logic is necessary for
> correct inner-map handling), so also amend check_kfunc_args() to
> match what helpers functions check is doing.
> 
> Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
> ---
>  kernel/bpf/verifier.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 86 insertions(+)
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index deaf2e1ab690..112faf2cd7e9 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -332,6 +332,10 @@ struct bpf_kfunc_call_arg_meta {
>  		u8 spi;
>  		u8 frameno;
>  	} iter;
> +	struct {
> +		struct bpf_map *ptr;
> +		int uid;
> +	} map;
>  	u64 mem_size;
>  };
>  
> @@ -7598,6 +7602,43 @@ static int process_timer_func(struct bpf_verifier_env *env, int regno,
>  	return 0;
>  }
>  
> +static int process_wq_func(struct bpf_verifier_env *env, int regno,
> +			   struct bpf_kfunc_call_arg_meta *meta)
> +{
> +	struct bpf_reg_state *regs = cur_regs(env), *reg = &regs[regno];
> +	bool is_const = tnum_is_const(reg->var_off);
> +	struct bpf_map *map = reg->map_ptr;
> +	u64 val = reg->var_off.value;
> +
> +	if (!is_const) {
> +		verbose(env,
> +			"R%d doesn't have constant offset. bpf_wq has to be at the constant offset\n",
> +			regno);
> +		return -EINVAL;
> +	}

This check is unnecessary.
Before calling into this function it goes as:
switch (kf_arg_type) {
case KF_ARG_PTR_TO_MAP:
    ... your new checks ...
    fallthrough;
case KF_ARG_PTR_TO_CTX:
    arg_type |= OBJ_RELEASE;
    break;

check_func_arg_reg_off()
  if (arg_type_is_release(arg_type)
      return __check_ptr_off_reg() 
      // that does offset checks

> +	if (!map->btf) {
> +		verbose(env, "map '%s' has to have BTF in order to use bpf_wq\n",
> +			map->name);
> +		return -EINVAL;
> +	}
> +	if (!btf_record_has_field(map->record, BPF_WORKQUEUE)) {
> +		verbose(env, "map '%s' has no valid bpf_wq\n", map->name);
> +		return -EINVAL;
> +	}

above two are also unnecessary. wq_off will be negative otherwise.

> +	if (map->record->wq_off != val + reg->off) {
> +		verbose(env, "off %lld doesn't point to 'struct bpf_wq' that is at %d\n",
> +			val + reg->off, map->record->wq_off);
> +		return -EINVAL;
> +	}

only this one is needed, but I need to study the rest of the patches.

> +	if (meta->map.ptr) {
> +		verbose(env, "verifier bug. Two map pointers in a workqueue helper\n");
> +		return -EFAULT;
> +	}

this was checked already as well.

> +	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)
>  {
> @@ -10843,6 +10884,7 @@ enum {
>  	KF_ARG_LIST_NODE_ID,
>  	KF_ARG_RB_ROOT_ID,
>  	KF_ARG_RB_NODE_ID,
> +	KF_ARG_WORKQUEUE_ID,
>  };
>  
>  BTF_ID_LIST(kf_arg_btf_ids)
> @@ -10851,6 +10893,7 @@ BTF_ID(struct, bpf_list_head)
>  BTF_ID(struct, bpf_list_node)
>  BTF_ID(struct, bpf_rb_root)
>  BTF_ID(struct, bpf_rb_node)
> +BTF_ID(struct, bpf_wq)
>  
>  static bool __is_kfunc_ptr_arg_type(const struct btf *btf,
>  				    const struct btf_param *arg, int type)
> @@ -10894,6 +10937,11 @@ static bool is_kfunc_arg_rbtree_node(const struct btf *btf, const struct btf_par
>  	return __is_kfunc_ptr_arg_type(btf, arg, KF_ARG_RB_NODE_ID);
>  }
>  
> +static bool is_kfunc_arg_wq(const struct btf *btf, const struct btf_param *arg)
> +{
> +	return __is_kfunc_ptr_arg_type(btf, arg, KF_ARG_WORKQUEUE_ID);
> +}
> +
>  static bool is_kfunc_arg_callback(struct bpf_verifier_env *env, const struct btf *btf,
>  				  const struct btf_param *arg)
>  {
> @@ -10963,6 +11011,7 @@ enum kfunc_ptr_arg_type {
>  	KF_ARG_PTR_TO_NULL,
>  	KF_ARG_PTR_TO_CONST_STR,
>  	KF_ARG_PTR_TO_MAP,
> +	KF_ARG_PTR_TO_WORKQUEUE,
>  };
>  
>  enum special_kfunc_type {
> @@ -11119,6 +11168,9 @@ get_kfunc_ptr_arg_type(struct bpf_verifier_env *env,
>  	if (is_kfunc_arg_map(meta->btf, &args[argno]))
>  		return KF_ARG_PTR_TO_MAP;
>  
> +	if (is_kfunc_arg_wq(meta->btf, &args[argno]))
> +		return KF_ARG_PTR_TO_WORKQUEUE;
> +
>  	if ((base_type(reg->type) == PTR_TO_BTF_ID || reg2btf_ids[base_type(reg->type)])) {
>  		if (!btf_type_is_struct(ref_t)) {
>  			verbose(env, "kernel function %s args#%d pointer type %s %s is not supported\n",
> @@ -11720,6 +11772,30 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
>  		case KF_ARG_PTR_TO_NULL:
>  			continue;
>  		case KF_ARG_PTR_TO_MAP:
> +			if (meta->map.ptr) {
> +				/* Use map_uid (which is unique id of inner map) to reject:
> +				 * inner_map1 = bpf_map_lookup_elem(outer_map, key1)
> +				 * inner_map2 = bpf_map_lookup_elem(outer_map, key2)
> +				 * if (inner_map1 && inner_map2) {
> +				 *     timer = bpf_map_lookup_elem(inner_map1);
> +				 *     if (timer)
> +				 *         // mismatch would have been allowed
> +				 *         bpf_timer_init(timer, inner_map2);

adjust the comment too. Here it can only be wq.

> +				 * }
> +				 *
> +				 * Comparing map_ptr is enough to distinguish normal and outer maps.
> +				 */
> +				if (meta->map.ptr != reg->map_ptr ||
> +				    meta->map.uid != reg->map_uid) {

let's also gate this check with map->record->wq_off.
Otherwise it will trigger for maps that don't have wq field.

Copy paste is a bit unfortunate, but fine. We can clean it up later.

> +					verbose(env,
> +						"workqueue pointer in R1 map_uid=%d doesn't match map pointer in R2 map_uid=%d\n",
> +						meta->map.uid, reg->map_uid);
> +					return -EINVAL;
> +				}
> +			}
> +			meta->map.ptr = reg->map_ptr;
> +			meta->map.uid = reg->map_uid;
> +			fallthrough;
>  		case KF_ARG_PTR_TO_ALLOC_BTF_ID:
>  		case KF_ARG_PTR_TO_BTF_ID:
>  			if (!is_kfunc_trusted_args(meta) && !is_kfunc_rcu(meta))
> @@ -11752,6 +11828,7 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
>  		case KF_ARG_PTR_TO_CALLBACK:
>  		case KF_ARG_PTR_TO_REFCOUNTED_KPTR:
>  		case KF_ARG_PTR_TO_CONST_STR:
> +		case KF_ARG_PTR_TO_WORKQUEUE:
>  			/* Trusted by default */
>  			break;
>  		default:
> @@ -12038,6 +12115,15 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
>  			if (ret)
>  				return ret;
>  			break;
> +		case KF_ARG_PTR_TO_WORKQUEUE:
> +			if (reg->type != PTR_TO_MAP_VALUE) {
> +				verbose(env, "arg#%d doesn't point to a map value\n", i);
> +				return -EINVAL;
> +			}
> +			ret = process_wq_func(env, regno, meta);
> +			if (ret < 0)
> +				return ret;
> +			break;
>  		}
>  	}
>  
> 
> -- 
> 2.44.0
>