On Wed, Sep 09, 2020 at 06:11:48PM +0100, Lorenz Bauer wrote:
> Function prototypes using ARG_PTR_TO_BTF_ID currently use two ways to signal
> which BTF IDs are acceptable. First, bpf_func_proto.btf_id is an array of
> IDs, one for each argument. This array is only accessed up to the highest
> numbered argument that uses ARG_PTR_TO_BTF_ID and may therefore be less than
> five arguments long. It usually points at a BTF_ID_LIST. Second, check_btf_id
> is a function pointer that is called by the verifier if present. It gets the
> actual BTF ID of the register, and the argument number we're currently checking.
> It turns out that the only user check_arg_btf_id ignores the argument, and is
> simply used to check whether the BTF ID has a struct sock_common at it's start.
>
> Replace both of these mechanisms with an explicit BTF ID for each argument
> in a function proto. Thanks to btf_struct_ids_match this is very flexible:
> check_arg_btf_id can be replaced by requiring struct sock_common.
[ ... ]
> BPF_CALL_4(bpf_get_stack_pe, struct bpf_perf_event_data_kern *, ctx,
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index c997f81c500b..7182c6e3eada 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -238,7 +238,6 @@ struct bpf_call_arg_meta {
> u64 msize_max_value;
> int ref_obj_id;
> int func_id;
> - u32 btf_id;
> };
>
> struct btf *btf_vmlinux;
> @@ -4002,29 +4001,23 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> goto err_type;
> }
> } else if (arg_type == ARG_PTR_TO_BTF_ID) {
> - bool ids_match = false;
> + const u32 *btf_id = fn->arg_btf_id[arg];
>
> expected_type = PTR_TO_BTF_ID;
> if (type != expected_type)
> goto err_type;
> - if (!fn->check_btf_id) {
> - if (reg->btf_id != meta->btf_id) {
> - ids_match = btf_struct_ids_match(&env->log, reg->off, reg->btf_id,
> - meta->btf_id);
> - if (!ids_match) {
> - verbose(env, "Helper has type %s got %s in R%d\n",
> - kernel_type_name(meta->btf_id),
> - kernel_type_name(reg->btf_id), regno);
> - return -EACCES;
> - }
> - }
> - } else if (!fn->check_btf_id(reg->btf_id, arg)) {
> - verbose(env, "Helper does not support %s in R%d\n",
> - kernel_type_name(reg->btf_id), regno);
>
> + if (!btf_id) {
> + verbose(env, "verifier internal error: missing BTF ID\n");
check_func_proto() could be a better place for this check.
> + return -EFAULT;
> + }
> +
> + if (!btf_struct_ids_match(&env->log, reg->off, reg->btf_id, *btf_id)) {
> + verbose(env, "R%d has incompatible type %s\n", regno,
> + kernel_type_name(reg->btf_id));
> return -EACCES;
> }
> - if ((reg->off && !ids_match) || !tnum_is_const(reg->var_off) || reg->var_off.value) {
> + if (!tnum_is_const(reg->var_off) || reg->var_off.value) {
Removing "(reg->off && !ids_match)" looks fine to me since it is
checked in btf_struct_ids_match(). Just want to highlight here
to get more attention.
> verbose(env, "R%d is a pointer to in-kernel struct with non-zero offset\n",
> regno);
> return -EACCES;
> @@ -4892,11 +4885,6 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn
> meta.func_id = func_id;
> /* check args */
> for (i = 0; i < 5; i++) {
> - if (!fn->check_btf_id) {
> - err = btf_resolve_helper_id(&env->log, fn, i);
> - if (err > 0)
> - meta.btf_id = err;
> - }
> err = check_func_arg(env, i, &meta, fn);
> if (err)
> return err;
[ ... ]
> diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c
> index a0d1a3265b71..442a34a7ee2b 100644
> --- a/net/core/bpf_sk_storage.c
> +++ b/net/core/bpf_sk_storage.c
> @@ -357,6 +357,7 @@ const struct bpf_func_proto bpf_sk_storage_get_proto = {
> .ret_type = RET_PTR_TO_MAP_VALUE_OR_NULL,
> .arg1_type = ARG_CONST_MAP_PTR,
> .arg2_type = ARG_PTR_TO_SOCKET,
> + .arg2_btf_id = &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON],
This change is not needed. It is not taking ARG_PTR_TO_BTF_ID.
> .arg3_type = ARG_PTR_TO_MAP_VALUE_OR_NULL,
> .arg4_type = ARG_ANYTHING,
> };
> @@ -377,21 +378,18 @@ const struct bpf_func_proto bpf_sk_storage_delete_proto = {
> .ret_type = RET_INTEGER,
> .arg1_type = ARG_CONST_MAP_PTR,
> .arg2_type = ARG_PTR_TO_SOCKET,
> + .arg2_btf_id = &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON],
Same here.
> };
>
> -BTF_ID_LIST(sk_storage_btf_ids)
> -BTF_ID_UNUSED
> -BTF_ID(struct, sock)
> -
> const struct bpf_func_proto sk_storage_get_btf_proto = {
> .func = bpf_sk_storage_get,
> .gpl_only = false,
> .ret_type = RET_PTR_TO_MAP_VALUE_OR_NULL,
> .arg1_type = ARG_CONST_MAP_PTR,
> .arg2_type = ARG_PTR_TO_BTF_ID,
> + .arg2_btf_id = &btf_sock_ids[BTF_SOCK_TYPE_SOCK],
+1 in reusing btf_sock_ids.
Others lgtm.
