| Message ID | 20220402015826.3941317-3-joannekoong@fb.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | Dynamic pointers | expand |
On Sat, Apr 02, 2022 at 07:28:21AM IST, Joanne Koong wrote: > From: Joanne Koong <joannelkoong@gmail.com> > > Currently, we hardcode in the verifier which functions are release > functions. We have no way of differentiating which argument is the one > to be released (we assume it will always be the first argument). > > This patch adds MEM_RELEASE as a bpf_type_flag. This allows us to > determine which argument in the function needs to be released, and > removes having to hardcode a list of release functions into the > verifier. > > Please note that currently, we only support one release argument in a > helper function. In the future, if/when we need to support several > release arguments within the function, MEM_RELEASE is necessary > since there needs to be a way of differentiating which arguments are the > release ones. > > In the near future, MEM_RELEASE will be used by dynptr helper functions > such as bpf_free. > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com> > --- > include/linux/bpf.h | 4 +++- > include/linux/bpf_verifier.h | 3 +-- > kernel/bpf/btf.c | 3 ++- > kernel/bpf/ringbuf.c | 4 ++-- > kernel/bpf/verifier.c | 42 ++++++++++++++++++------------------ > net/core/filter.c | 2 +- > 6 files changed, 30 insertions(+), 28 deletions(-) > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > index 6f2558da9d4a..cb9f42866cde 100644 > --- a/include/linux/bpf.h > +++ b/include/linux/bpf.h > @@ -344,7 +344,9 @@ enum bpf_type_flag { > > MEM_UNINIT = BIT(5 + BPF_BASE_TYPE_BITS), > > - __BPF_TYPE_LAST_FLAG = MEM_UNINIT, > + MEM_RELEASE = BIT(6 + BPF_BASE_TYPE_BITS), > + > + __BPF_TYPE_LAST_FLAG = MEM_RELEASE, > }; > > /* Max number of base types. */ > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h > index c1fc4af47f69..7a01adc9e13f 100644 > --- a/include/linux/bpf_verifier.h > +++ b/include/linux/bpf_verifier.h > @@ -523,8 +523,7 @@ int check_ptr_off_reg(struct bpf_verifier_env *env, > const struct bpf_reg_state *reg, int regno); > int check_func_arg_reg_off(struct bpf_verifier_env *env, > const struct bpf_reg_state *reg, int regno, > - enum bpf_arg_type arg_type, > - bool is_release_func); > + enum bpf_arg_type arg_type, bool arg_release); > int check_kfunc_mem_size_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg, > u32 regno); > int check_mem_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg, > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > index 0918a39279f6..e5b765a84aec 100644 > --- a/kernel/bpf/btf.c > +++ b/kernel/bpf/btf.c > @@ -5830,7 +5830,8 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, > ref_t = btf_type_skip_modifiers(btf, t->type, &ref_id); > ref_tname = btf_name_by_offset(btf, ref_t->name_off); > > - ret = check_func_arg_reg_off(env, reg, regno, ARG_DONTCARE, rel); > + ret = check_func_arg_reg_off(env, reg, regno, ARG_DONTCARE, > + rel && reg->ref_obj_id); > if (ret < 0) > return ret; > > diff --git a/kernel/bpf/ringbuf.c b/kernel/bpf/ringbuf.c > index 710ba9de12ce..a723aa484ce4 100644 > --- a/kernel/bpf/ringbuf.c > +++ b/kernel/bpf/ringbuf.c > @@ -404,7 +404,7 @@ BPF_CALL_2(bpf_ringbuf_submit, void *, sample, u64, flags) > const struct bpf_func_proto bpf_ringbuf_submit_proto = { > .func = bpf_ringbuf_submit, > .ret_type = RET_VOID, > - .arg1_type = ARG_PTR_TO_ALLOC_MEM, > + .arg1_type = ARG_PTR_TO_ALLOC_MEM | MEM_RELEASE, > .arg2_type = ARG_ANYTHING, > }; > > @@ -417,7 +417,7 @@ BPF_CALL_2(bpf_ringbuf_discard, void *, sample, u64, flags) > const struct bpf_func_proto bpf_ringbuf_discard_proto = { > .func = bpf_ringbuf_discard, > .ret_type = RET_VOID, > - .arg1_type = ARG_PTR_TO_ALLOC_MEM, > + .arg1_type = ARG_PTR_TO_ALLOC_MEM | MEM_RELEASE, > .arg2_type = ARG_ANYTHING, > }; > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 90280d5666be..80e53303713e 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -471,15 +471,12 @@ static bool type_may_be_null(u32 type) > return type & PTR_MAYBE_NULL; > } > > -/* Determine whether the function releases some resources allocated by another > - * function call. The first reference type argument will be assumed to be > - * released by release_reference(). > +/* Determine whether the type releases some resources allocated by a > + * previous function call. > */ > -static bool is_release_function(enum bpf_func_id func_id) > +static bool type_is_release_mem(u32 type) > { > - return func_id == BPF_FUNC_sk_release || > - func_id == BPF_FUNC_ringbuf_submit || > - func_id == BPF_FUNC_ringbuf_discard; > + return type & MEM_RELEASE; > } > > static bool may_be_acquire_function(enum bpf_func_id func_id) > @@ -5364,13 +5361,11 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno, > > int check_func_arg_reg_off(struct bpf_verifier_env *env, > const struct bpf_reg_state *reg, int regno, > - enum bpf_arg_type arg_type, > - bool is_release_func) > + enum bpf_arg_type arg_type, bool arg_release) > { > - bool fixed_off_ok = false, release_reg; > - enum bpf_reg_type type = reg->type; > + bool fixed_off_ok = false; > > - switch ((u32)type) { > + switch ((u32)reg->type) { > case SCALAR_VALUE: > /* Pointer types where reg offset is explicitly allowed: */ > case PTR_TO_PACKET: > @@ -5393,18 +5388,15 @@ int check_func_arg_reg_off(struct bpf_verifier_env *env, > * fixed offset. > */ > case PTR_TO_BTF_ID: > - /* When referenced PTR_TO_BTF_ID is passed to release function, > - * it's fixed offset must be 0. We rely on the property that > - * only one referenced register can be passed to BPF helpers and > - * kfuncs. In the other cases, fixed offset can be non-zero. > + /* If a referenced PTR_TO_BTF_ID will be released, its fixed offset > + * must be 0. > */ > - release_reg = is_release_func && reg->ref_obj_id; > - if (release_reg && reg->off) { > + if (arg_release && reg->off) { > verbose(env, "R%d must have zero offset when passed to release func\n", > regno); > return -EINVAL; > } > - /* For release_reg == true, fixed_off_ok must be false, but we > + /* For arg_release == true, fixed_off_ok must be false, but we > * already checked and rejected reg->off != 0 above, so set to > * true to allow fixed offset for all other cases. > */ > @@ -5424,6 +5416,7 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, > struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno]; > enum bpf_arg_type arg_type = fn->arg_type[arg]; > enum bpf_reg_type type = reg->type; > + bool arg_release; > int err = 0; > > if (arg_type == ARG_DONTCARE) > @@ -5464,7 +5457,14 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, > if (err) > return err; > > - err = check_func_arg_reg_off(env, reg, regno, arg_type, is_release_function(meta->func_id)); > + arg_release = type_is_release_mem(arg_type); > + if (arg_release && !reg->ref_obj_id) { > + verbose(env, "R%d arg #%d is an unacquired reference and hence cannot be released\n", > + regno, arg + 1); > + return -EINVAL; > + } > + > + err = check_func_arg_reg_off(env, reg, regno, arg_type, arg_release); > if (err) > return err; > > @@ -6693,7 +6693,7 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn > return err; > } > > - if (is_release_function(func_id)) { > + if (meta.ref_obj_id) { The meta.ref_obj_id field is set unconditionally whenever we see a reg->ref_obj_id, e.g. when we pass a refcounted argument to non-release function. Wouldn't making this conditional only on meta.ref_obj_id lead to release of that register now? Or did I miss some change above which prevents this case? To make things clear, I'm talking of this sequence: p = acquire(); helper_foo(p); // meta.ref_obj_id would be set, and p is released release(p); // error, as p.ref_obj_id has no reference state Besides, in my series this PTR_RELEASE / MEM_RELEASE tagging is only needed because the release function can take a NULL pointer, so we need to know the register of the argument to be released, and then make sure it is refcounted, otherwise it must be NULL (and whether NULL is permitted or not is checked earlier during argument checks). That doesn't seem to be true for bpf_free in your series, as it can only take ARG_PTR_TO_DYNPTR (but maybe it should also set PTR_MAYBE_NULL). > err = release_reference(env, meta.ref_obj_id); > if (err) { > verbose(env, "func %s#%d reference has not been acquired before\n", > diff --git a/net/core/filter.c b/net/core/filter.c > index 9aafec3a09ed..a935ce7a63bc 100644 > --- a/net/core/filter.c > +++ b/net/core/filter.c > @@ -6621,7 +6621,7 @@ static const struct bpf_func_proto bpf_sk_release_proto = { > .func = bpf_sk_release, > .gpl_only = false, > .ret_type = RET_INTEGER, > - .arg1_type = ARG_PTR_TO_BTF_ID_SOCK_COMMON, > + .arg1_type = ARG_PTR_TO_BTF_ID_SOCK_COMMON | MEM_RELEASE, > }; > > BPF_CALL_5(bpf_xdp_sk_lookup_udp, struct xdp_buff *, ctx, > -- > 2.30.2 > -- Kartikeya
On Mon, Apr 4, 2022 at 12:34 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > On Sat, Apr 02, 2022 at 07:28:21AM IST, Joanne Koong wrote: > > From: Joanne Koong <joannelkoong@gmail.com> > > > > Currently, we hardcode in the verifier which functions are release > > functions. We have no way of differentiating which argument is the one > > to be released (we assume it will always be the first argument). > > > > This patch adds MEM_RELEASE as a bpf_type_flag. This allows us to > > determine which argument in the function needs to be released, and > > removes having to hardcode a list of release functions into the > > verifier. > > > > Please note that currently, we only support one release argument in a > > helper function. In the future, if/when we need to support several > > release arguments within the function, MEM_RELEASE is necessary > > since there needs to be a way of differentiating which arguments are the > > release ones. > > > > In the near future, MEM_RELEASE will be used by dynptr helper functions > > such as bpf_free. > > > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com> > > --- [...] > > @@ -6693,7 +6693,7 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn > > return err; > > } > > > > - if (is_release_function(func_id)) { > > + if (meta.ref_obj_id) { > > The meta.ref_obj_id field is set unconditionally whenever we see a > reg->ref_obj_id, e.g. when we pass a refcounted argument to non-release > function. Wouldn't making this conditional only on meta.ref_obj_id lead to > release of that register now? Or did I miss some change above which prevents > this case? > Yes, unfortunately you are right. This wouldn't work for the cases where a refcounted arg is passed to a non-release function, since that also sets the meta.ref_obj_id. Thanks for catching this! > To make things clear, I'm talking of this sequence: > > p = acquire(); > helper_foo(p); // meta.ref_obj_id would be set, and p is released > release(p); // error, as p.ref_obj_id has no reference state > > Besides, in my series this PTR_RELEASE / MEM_RELEASE tagging is only needed > because the release function can take a NULL pointer, so we need to know the > register of the argument to be released, and then make sure it is refcounted, > otherwise it must be NULL (and whether NULL is permitted or not is checked > earlier during argument checks). That doesn't seem to be true for bpf_free in > your series, as it can only take ARG_PTR_TO_DYNPTR (but maybe it should also > set PTR_MAYBE_NULL). > In the dynptr case, there will be several release-type functions (eg bpf_free, bpf_ringbuf_discard, bpf_ringbuf_submit). The motivation behind this patch was to have some way of signifying this instead of having to specify in the verifier the particular functions. Please let me know if this addresses your comment or if there's something in between the lines in your reply that I'm missing > > err = release_reference(env, meta.ref_obj_id); > > if (err) { > > verbose(env, "func %s#%d reference has not been acquired before\n", > > diff --git a/net/core/filter.c b/net/core/filter.c > > index 9aafec3a09ed..a935ce7a63bc 100644 > > --- a/net/core/filter.c > > +++ b/net/core/filter.c > > @@ -6621,7 +6621,7 @@ static const struct bpf_func_proto bpf_sk_release_proto = { > > .func = bpf_sk_release, > > .gpl_only = false, > > .ret_type = RET_INTEGER, > > - .arg1_type = ARG_PTR_TO_BTF_ID_SOCK_COMMON, > > + .arg1_type = ARG_PTR_TO_BTF_ID_SOCK_COMMON | MEM_RELEASE, > > }; > > > > BPF_CALL_5(bpf_xdp_sk_lookup_udp, struct xdp_buff *, ctx, > > -- > > 2.30.2 > > > > -- > Kartikeya
On Fri, Apr 1, 2022 at 7:00 PM Joanne Koong <joannekoong@fb.com> wrote: > > From: Joanne Koong <joannelkoong@gmail.com> > > Currently, we hardcode in the verifier which functions are release > functions. We have no way of differentiating which argument is the one > to be released (we assume it will always be the first argument). > > This patch adds MEM_RELEASE as a bpf_type_flag. This allows us to > determine which argument in the function needs to be released, and > removes having to hardcode a list of release functions into the > verifier. > > Please note that currently, we only support one release argument in a > helper function. In the future, if/when we need to support several > release arguments within the function, MEM_RELEASE is necessary > since there needs to be a way of differentiating which arguments are the > release ones. > > In the near future, MEM_RELEASE will be used by dynptr helper functions > such as bpf_free. > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com> > --- > include/linux/bpf.h | 4 +++- > include/linux/bpf_verifier.h | 3 +-- > kernel/bpf/btf.c | 3 ++- > kernel/bpf/ringbuf.c | 4 ++-- > kernel/bpf/verifier.c | 42 ++++++++++++++++++------------------ > net/core/filter.c | 2 +- > 6 files changed, 30 insertions(+), 28 deletions(-) > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > index 6f2558da9d4a..cb9f42866cde 100644 > --- a/include/linux/bpf.h > +++ b/include/linux/bpf.h > @@ -344,7 +344,9 @@ enum bpf_type_flag { > > MEM_UNINIT = BIT(5 + BPF_BASE_TYPE_BITS), > > - __BPF_TYPE_LAST_FLAG = MEM_UNINIT, > + MEM_RELEASE = BIT(6 + BPF_BASE_TYPE_BITS), "MEM_" part seems a bit too specific, it's not necessarily (just) about memory, it's more generally about "releasing resources" in general, right? ARG_RELEASE or OBJ_RELEASE maybe? > + > + __BPF_TYPE_LAST_FLAG = MEM_RELEASE, > }; > [...] > -/* Determine whether the function releases some resources allocated by another > - * function call. The first reference type argument will be assumed to be > - * released by release_reference(). > +/* Determine whether the type releases some resources allocated by a > + * previous function call. > */ > -static bool is_release_function(enum bpf_func_id func_id) > +static bool type_is_release_mem(u32 type) > { > - return func_id == BPF_FUNC_sk_release || > - func_id == BPF_FUNC_ringbuf_submit || > - func_id == BPF_FUNC_ringbuf_discard; > + return type & MEM_RELEASE; > } > same skepticism regarding the need for this helper function, just makes grepping code slightly harder > static bool may_be_acquire_function(enum bpf_func_id func_id) [...]
diff --git a/include/linux/bpf.h b/include/linux/bpf.h index 6f2558da9d4a..cb9f42866cde 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -344,7 +344,9 @@ enum bpf_type_flag { MEM_UNINIT = BIT(5 + BPF_BASE_TYPE_BITS), - __BPF_TYPE_LAST_FLAG = MEM_UNINIT, + MEM_RELEASE = BIT(6 + BPF_BASE_TYPE_BITS), + + __BPF_TYPE_LAST_FLAG = MEM_RELEASE, }; /* Max number of base types. */ diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h index c1fc4af47f69..7a01adc9e13f 100644 --- a/include/linux/bpf_verifier.h +++ b/include/linux/bpf_verifier.h @@ -523,8 +523,7 @@ int check_ptr_off_reg(struct bpf_verifier_env *env, const struct bpf_reg_state *reg, int regno); int check_func_arg_reg_off(struct bpf_verifier_env *env, const struct bpf_reg_state *reg, int regno, - enum bpf_arg_type arg_type, - bool is_release_func); + enum bpf_arg_type arg_type, bool arg_release); int check_kfunc_mem_size_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg, u32 regno); int check_mem_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg, diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c index 0918a39279f6..e5b765a84aec 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c @@ -5830,7 +5830,8 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, ref_t = btf_type_skip_modifiers(btf, t->type, &ref_id); ref_tname = btf_name_by_offset(btf, ref_t->name_off); - ret = check_func_arg_reg_off(env, reg, regno, ARG_DONTCARE, rel); + ret = check_func_arg_reg_off(env, reg, regno, ARG_DONTCARE, + rel && reg->ref_obj_id); if (ret < 0) return ret; diff --git a/kernel/bpf/ringbuf.c b/kernel/bpf/ringbuf.c index 710ba9de12ce..a723aa484ce4 100644 --- a/kernel/bpf/ringbuf.c +++ b/kernel/bpf/ringbuf.c @@ -404,7 +404,7 @@ BPF_CALL_2(bpf_ringbuf_submit, void *, sample, u64, flags) const struct bpf_func_proto bpf_ringbuf_submit_proto = { .func = bpf_ringbuf_submit, .ret_type = RET_VOID, - .arg1_type = ARG_PTR_TO_ALLOC_MEM, + .arg1_type = ARG_PTR_TO_ALLOC_MEM | MEM_RELEASE, .arg2_type = ARG_ANYTHING, }; @@ -417,7 +417,7 @@ BPF_CALL_2(bpf_ringbuf_discard, void *, sample, u64, flags) const struct bpf_func_proto bpf_ringbuf_discard_proto = { .func = bpf_ringbuf_discard, .ret_type = RET_VOID, - .arg1_type = ARG_PTR_TO_ALLOC_MEM, + .arg1_type = ARG_PTR_TO_ALLOC_MEM | MEM_RELEASE, .arg2_type = ARG_ANYTHING, }; diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 90280d5666be..80e53303713e 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -471,15 +471,12 @@ static bool type_may_be_null(u32 type) return type & PTR_MAYBE_NULL; } -/* Determine whether the function releases some resources allocated by another - * function call. The first reference type argument will be assumed to be - * released by release_reference(). +/* Determine whether the type releases some resources allocated by a + * previous function call. */ -static bool is_release_function(enum bpf_func_id func_id) +static bool type_is_release_mem(u32 type) { - return func_id == BPF_FUNC_sk_release || - func_id == BPF_FUNC_ringbuf_submit || - func_id == BPF_FUNC_ringbuf_discard; + return type & MEM_RELEASE; } static bool may_be_acquire_function(enum bpf_func_id func_id) @@ -5364,13 +5361,11 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno, int check_func_arg_reg_off(struct bpf_verifier_env *env, const struct bpf_reg_state *reg, int regno, - enum bpf_arg_type arg_type, - bool is_release_func) + enum bpf_arg_type arg_type, bool arg_release) { - bool fixed_off_ok = false, release_reg; - enum bpf_reg_type type = reg->type; + bool fixed_off_ok = false; - switch ((u32)type) { + switch ((u32)reg->type) { case SCALAR_VALUE: /* Pointer types where reg offset is explicitly allowed: */ case PTR_TO_PACKET: @@ -5393,18 +5388,15 @@ int check_func_arg_reg_off(struct bpf_verifier_env *env, * fixed offset. */ case PTR_TO_BTF_ID: - /* When referenced PTR_TO_BTF_ID is passed to release function, - * it's fixed offset must be 0. We rely on the property that - * only one referenced register can be passed to BPF helpers and - * kfuncs. In the other cases, fixed offset can be non-zero. + /* If a referenced PTR_TO_BTF_ID will be released, its fixed offset + * must be 0. */ - release_reg = is_release_func && reg->ref_obj_id; - if (release_reg && reg->off) { + if (arg_release && reg->off) { verbose(env, "R%d must have zero offset when passed to release func\n", regno); return -EINVAL; } - /* For release_reg == true, fixed_off_ok must be false, but we + /* For arg_release == true, fixed_off_ok must be false, but we * already checked and rejected reg->off != 0 above, so set to * true to allow fixed offset for all other cases. */ @@ -5424,6 +5416,7 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno]; enum bpf_arg_type arg_type = fn->arg_type[arg]; enum bpf_reg_type type = reg->type; + bool arg_release; int err = 0; if (arg_type == ARG_DONTCARE) @@ -5464,7 +5457,14 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, if (err) return err; - err = check_func_arg_reg_off(env, reg, regno, arg_type, is_release_function(meta->func_id)); + arg_release = type_is_release_mem(arg_type); + if (arg_release && !reg->ref_obj_id) { + verbose(env, "R%d arg #%d is an unacquired reference and hence cannot be released\n", + regno, arg + 1); + return -EINVAL; + } + + err = check_func_arg_reg_off(env, reg, regno, arg_type, arg_release); if (err) return err; @@ -6693,7 +6693,7 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn return err; } - if (is_release_function(func_id)) { + if (meta.ref_obj_id) { err = release_reference(env, meta.ref_obj_id); if (err) { verbose(env, "func %s#%d reference has not been acquired before\n", diff --git a/net/core/filter.c b/net/core/filter.c index 9aafec3a09ed..a935ce7a63bc 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -6621,7 +6621,7 @@ static const struct bpf_func_proto bpf_sk_release_proto = { .func = bpf_sk_release, .gpl_only = false, .ret_type = RET_INTEGER, - .arg1_type = ARG_PTR_TO_BTF_ID_SOCK_COMMON, + .arg1_type = ARG_PTR_TO_BTF_ID_SOCK_COMMON | MEM_RELEASE, }; BPF_CALL_5(bpf_xdp_sk_lookup_udp, struct xdp_buff *, ctx,