Re: [PATCH bpf-next v5 2/6] bpf/verifier: add bpf_timer as a kfunc capable type

On Fri, 22 Mar 2024 at 15:57, Benjamin Tissoires bentiss@kernel.org wrote:

We need to extend the bpf_timer API, but the way forward relies on kfuncs. So make bpf_timer known for kfuncs from the verifier PoV

Signed-off-by: Benjamin Tissoires bentiss@kernel.org

---

changes in v5:

• also check for the reg offset

changes in v4:

• enforce KF_ARG_PTR_TO_TIMER to be of type PTR_TO_MAP_VALUE

new in v3 (split from v2 02/10)

kernel/bpf/verifier.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 63749ad5ac6b..24a604e26ec7 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -10826,6 +10826,7 @@ enum { KF_ARG_LIST_NODE_ID, KF_ARG_RB_ROOT_ID, KF_ARG_RB_NODE_ID,

•   KF_ARG_TIMER_ID,
  

};

BTF_ID_LIST(kf_arg_btf_ids) @@ -10834,6 +10835,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_timer_kern)

static bool __is_kfunc_ptr_arg_type(const struct btf *btf, const struct btf_param *arg, int type) @@ -10877,6 +10879,12 @@ 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_timer(const struct btf *btf, const struct btf_param *arg) +{

•   bool ret = __is_kfunc_ptr_arg_type(btf, arg, KF_ARG_TIMER_ID);
  

•   return ret;
  

+}

static bool is_kfunc_arg_callback(struct bpf_verifier_env *env, const struct btf *btf, const struct btf_param *arg) { @@ -10946,6 +10954,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_TIMER,
  

};

enum special_kfunc_type { @@ -11102,6 +11111,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_timer(meta->btf, &args[argno]))
  

•           return KF_ARG_PTR_TO_TIMER;
  

•   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",
  

@@ -11735,6 +11747,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_TIMER:
                    /* Trusted by default */
                    break;
            default:
  

@@ -12021,6 +12034,16 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_ if (ret) return ret; break;

•           case KF_ARG_PTR_TO_TIMER:
  

•                   if (reg->type != PTR_TO_MAP_VALUE) {
  

•                           verbose(env, "arg#%d doesn't point to a map value\n", i);
  

•                           return -EINVAL;
  

•                   }
  

•                   if (reg->off) {
  

•                           verbose(env, "arg#%d offset can not be greater than 0\n", i);
  

•                           return -EINVAL;
  

•                   }
  

This won't be correct. You don't really check whether the timer exists at reg->off (and if you did, this would still restrict it to 0 offset, and not check the variable offset which would be non-zero). What I would suggest is calling process_timer_func (see how dynptr calls the same underlying process_dynptr_func to enforce type invariants). This would allow sharing the same checks and avoid bugs from creeping in. It does all checks wrt constant/variable offset and looking up the timer field offset and matching it against the one in the pointer.

•                   break;
            }
    }
  
  
  

-- 2.44.0