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;
+ }
+ break;
}
}
--
2.44.0On 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
>
>
On Fri, Mar 22, 2024 at 9:31 AM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> 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.
Observation is correct. The patch is buggy,
but the suggestion to follow process_dynptr_func() will lead
to unnecessary complexity.
dynptr-s are on stack with plenty of extra checks.
In this case bpf_timer is in map_value.
Much simpler is to follow KF_ARG_PTR_TO_MAP approach.
Set ref_id, ref_t, ref_tname to bpf_timer and
process_kf_arg_ptr_to_btf_id() should work as-is.
You still need
+BTF_ID(struct, bpf_timer_kern)
to recognize that argument in a kfunc,
but the selftests will be using 'struct bpf_timer t',
so KF_ARG_PTR_TO_TIMER would need to match against 'struct bpf_timer'
and not against 'struct bpf_timer_kern'.
On Sun, 24 Mar 2024 at 04:53, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Fri, Mar 22, 2024 at 9:31 AM Kumar Kartikeya Dwivedi
> <memxor@gmail.com> wrote:
> >
> > 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.
>
> Observation is correct. The patch is buggy,
> but the suggestion to follow process_dynptr_func() will lead
> to unnecessary complexity.
> dynptr-s are on stack with plenty of extra checks.
The suggestion was to call process_timer_func, not process_dynptr_func.
> In this case bpf_timer is in map_value.
> Much simpler is to follow KF_ARG_PTR_TO_MAP approach.
What I meant by the example was that dynptr handling does the same
thing for kfuncs and helpers (using the same function), so timer
arguments should do the same (i.e. use process_timer_func), which will
do all checks for constant offset (ensuring var_off is tnum_is_const)
and match it against btf_record->timer_off.
> [...]
On Sat, Mar 23, 2024 at 9:01 PM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> On Sun, 24 Mar 2024 at 04:53, Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Fri, Mar 22, 2024 at 9:31 AM Kumar Kartikeya Dwivedi
> > <memxor@gmail.com> wrote:
> > >
> > > 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.
> >
> > Observation is correct. The patch is buggy,
> > but the suggestion to follow process_dynptr_func() will lead
> > to unnecessary complexity.
> > dynptr-s are on stack with plenty of extra checks.
>
> The suggestion was to call process_timer_func, not process_dynptr_func.
>
> > In this case bpf_timer is in map_value.
> > Much simpler is to follow KF_ARG_PTR_TO_MAP approach.
>
> What I meant by the example was that dynptr handling does the same
> thing for kfuncs and helpers (using the same function), so timer
> arguments should do the same (i.e. use process_timer_func), which will
> do all checks for constant offset (ensuring var_off is tnum_is_const)
> and match it against btf_record->timer_off.
I don't follow. Please elaborate with a patch.
The var_off and off is a part of the bug, but it's not the biggest part of it.
On Sun, 24 Mar 2024 at 05:38, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Sat, Mar 23, 2024 at 9:01 PM Kumar Kartikeya Dwivedi
> <memxor@gmail.com> wrote:
> >
> > On Sun, 24 Mar 2024 at 04:53, Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Fri, Mar 22, 2024 at 9:31 AM Kumar Kartikeya Dwivedi
> > > <memxor@gmail.com> wrote:
> > > >
> > > > 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.
> > >
> > > Observation is correct. The patch is buggy,
> > > but the suggestion to follow process_dynptr_func() will lead
> > > to unnecessary complexity.
> > > dynptr-s are on stack with plenty of extra checks.
> >
> > The suggestion was to call process_timer_func, not process_dynptr_func.
> >
> > > In this case bpf_timer is in map_value.
> > > Much simpler is to follow KF_ARG_PTR_TO_MAP approach.
> >
> > What I meant by the example was that dynptr handling does the same
> > thing for kfuncs and helpers (using the same function), so timer
> > arguments should do the same (i.e. use process_timer_func), which will
> > do all checks for constant offset (ensuring var_off is tnum_is_const)
> > and match it against btf_record->timer_off.
>
> I don't follow. Please elaborate with a patch.
> The var_off and off is a part of the bug, but it's not the biggest part of it.
Not compile tested.
From 8f8c79d2df299efc76b2bfb5932603ca3e738e8a Mon Sep 17 00:00:00 2001
From: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Date: Sun, 24 Mar 2024 04:49:29 +0000
Subject: [PATCH] bpf: Add support for KF_ARG_PTR_TO_TIMER
Not compile tested.
Introduce support for KF_ARG_PTR_TO_TIMER. The kfuncs will use bpf_timer
as argument and that will be recognized as timer argument by verifier.
bpf_timer_kern casting can happen inside kfunc, but using bpf_timer in
argument makes life easier for users who work with non-kern type in BPF
progs.
Fix up process_timer_func's meta argument usage (ignore if NULL) so that
we can share the same checks for helpers and kfuncs. meta argument is
only needed to ensure bpf_timer_init's timer and map arguments are
coming from the same map (map_uid logic is necessary for correct
inner-map handling).
No such concerns will be necessary for kfuncs as timer initialization
happens using helpers, hence pass NULL to process_timer_func from kfunc
argument handling code to ignore it.
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
kernel/bpf/verifier.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index ca6cacf7b42f..d89ba16b56e4 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -7568,12 +7568,16 @@ static int process_timer_func(struct bpf_verifier_env *env, int regno,
val + reg->off, map->record->timer_off);
return -EINVAL;
}
+ /* meta is only needed for bpf_timer_init to match timer and map */
+ if (!meta)
+ goto out;
if (meta->map_ptr) {
verbose(env, "verifier bug. Two map pointers in a timer helper\n");
return -EFAULT;
}
meta->map_uid = reg->map_uid;
meta->map_ptr = map;
+out:
return 0;
}
@@ -10826,6 +10830,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 +10839,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)
static bool __is_kfunc_ptr_arg_type(const struct btf *btf,
const struct btf_param *arg, int type)
@@ -10877,6 +10883,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_timer(const struct btf *btf, const struct btf_param *arg)
+{
+ return __is_kfunc_ptr_arg_type(btf, arg, KF_ARG_TIMER_ID);
+}
+
static bool is_kfunc_arg_callback(struct bpf_verifier_env *env, const struct btf *btf,
const struct btf_param *arg)
{
@@ -10946,6 +10957,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 +11114,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 +11750,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 +12037,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_TIMER:
+ if (reg->type != PTR_TO_MAP_VALUE) {
+ verbose(env, "arg#%d doesn't point to a map value\n", i);
+ return -EINVAL;
+ }
+ ret = process_timer_func(env, regno, NULL);
+ if (ret < 0)
+ return ret;
+ break;
}
}
--
2.43.0
On Sat, Mar 23, 2024 at 9:57 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > > > > > > > Observation is correct. The patch is buggy, > > > > but the suggestion to follow process_dynptr_func() will lead > > > > to unnecessary complexity. > > > > dynptr-s are on stack with plenty of extra checks. > > > > > > The suggestion was to call process_timer_func, not process_dynptr_func. > > > > > > > In this case bpf_timer is in map_value. > > > > Much simpler is to follow KF_ARG_PTR_TO_MAP approach. > > > > > > What I meant by the example was that dynptr handling does the same > > > thing for kfuncs and helpers (using the same function), so timer > > > arguments should do the same (i.e. use process_timer_func), which will > > > do all checks for constant offset (ensuring var_off is tnum_is_const) > > > and match it against btf_record->timer_off. > > > > I don't follow. Please elaborate with a patch. > > The var_off and off is a part of the bug, but it's not the biggest part of it. > > Not compile tested. I see. All makes sense to me. Benjamin, pls incorporate it in your set.
On Mar 24 2024, Alexei Starovoitov wrote: > On Sat, Mar 23, 2024 at 9:57 PM Kumar Kartikeya Dwivedi > <memxor@gmail.com> wrote: > > > > > > > > > > > Observation is correct. The patch is buggy, > > > > > but the suggestion to follow process_dynptr_func() will lead > > > > > to unnecessary complexity. > > > > > dynptr-s are on stack with plenty of extra checks. > > > > > > > > The suggestion was to call process_timer_func, not process_dynptr_func. > > > > > > > > > In this case bpf_timer is in map_value. > > > > > Much simpler is to follow KF_ARG_PTR_TO_MAP approach. > > > > > > > > What I meant by the example was that dynptr handling does the same > > > > thing for kfuncs and helpers (using the same function), so timer > > > > arguments should do the same (i.e. use process_timer_func), which will > > > > do all checks for constant offset (ensuring var_off is tnum_is_const) > > > > and match it against btf_record->timer_off. > > > > > > I don't follow. Please elaborate with a patch. > > > The var_off and off is a part of the bug, but it's not the biggest part of it. > > > > Not compile tested. Compiles just fine :) > > I see. All makes sense to me. > > Benjamin, > pls incorporate it in your set. > OK, done! I just had to revert to the following or KF_ARG_TIMER_ID was not recognized by the verifier: --- diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 7ee20e9d14bd..a5e147468ac8 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -10848,7 +10848,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) +BTF_ID(struct, bpf_timer_kern) static bool __is_kfunc_ptr_arg_type(const struct btf *btf, const struct btf_param *arg, int type) --- Cheers, Benjamin
© 2016 - 2026 Red Hat, Inc.