| Message ID | 20221020160721.4030492-2-davemarchevsky@fb.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | d1673304097c1f5b04e062cf62fb40200ef1546b |
| Delegated to: | BPF |
| Headers | show |
| Series | [v5,bpf-next,1/4] bpf: Allow ringbuf memory to be used as map key | expand |
On Thu, Oct 20, 2022 at 9:07 AM Dave Marchevsky <davemarchevsky@fb.com> wrote: > > After the previous patch, which added PTR_TO_MEM | MEM_ALLOC type > map_key_value_types, the only difference between map_key_value_types and > mem_types sets is PTR_TO_BUF and PTR_TO_MEM, which are in the latter set > but not the former. > > Helpers which expect ARG_PTR_TO_MAP_KEY or ARG_PTR_TO_MAP_VALUE > already effectively expect a valid blob of arbitrary memory that isn't > necessarily explicitly associated with a map. When validating a > PTR_TO_MAP_{KEY,VALUE} arg, the verifier expects meta->map_ptr to have > already been set, either by an earlier ARG_CONST_MAP_PTR arg, or custom > logic like that in process_timer_func or process_kptr_func. > > So let's get rid of map_key_value_types and just use mem_types for those > args. > > This has the effect of adding PTR_TO_BUF and PTR_TO_MEM to the set of > compatible types for ARG_PTR_TO_MAP_KEY and ARG_PTR_TO_MAP_VALUE. > > PTR_TO_BUF is used by various bpf_iter implementations to represent a > chunk of valid r/w memory in ctx args for iter prog. > > PTR_TO_MEM is used by networking, tracing, and ringbuf helpers to > represent a chunk of valid memory. The PTR_TO_MEM | MEM_ALLOC > type added in previous commmit is specific to ringbuf helpers. typo: s/commmit/commit/ (but not worth reposting just to fix this) btw, I have a strong desire to change PTR_TO_MEM | MEM_ALLOC into its own PTR_TO_RINGBUF_RECORD (or something less verbose), it's very confusing that "MEM_ALLOC" is very crucially *a ringbuf record* pointer. Can't be anything else, but name won't suggest this, we'll trip ourselves over this in the future. > Presence or absence of MEM_ALLOC doesn't change the validity of using > PTR_TO_MEM as a map_{key,val} input. > > Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com> > --- > v1 -> v5: lore.kernel.org/bpf/20220912101106.2765921-2-davemarchevsky@fb.com > > * This patch was dropped in v2 as I had no concrete usecase for > PTR_TO_BUF and PTR_TO_MEM w/o MEM_ALLOC. Andrii encouraged me to > re-add the patch as we both share desire to eventually cleanup all > these separate "valid chunk of memory" types. Starting to treat them > similarly is a good step in that direction. Yep, 100% agree. We should try to generalize code and types for conceptually similar things to make things a bit more manageable. As another example, seems like ARG_PTR_TO_MAP_KEY and ARG_PTR_TO_MAP_VALUE handling inside check_func_arg() is basically identical, we just need to pass meta->raw_mode = false for ARG_PTR_TO_MAP_KEY case to mark "read-only" operation. Something for future clean ups, though. This patch looks great, thanks! Acked-by: Andrii Nakryiko <andrii@kernel.org> > * A usecase for PTR_TO_BUF is now demonstrated in patch 4 of this > series. > * PTR_TO_MEM w/o MEM_ALLOC is returned by bpf_{this,per}_cpu_ptr > helpers via RET_PTR_TO_MEM_OR_BTF_ID, but in both cases the return > type is also tagged MEM_RDONLY, which map helpers don't currently > accept (see patch 4 summary). So no selftest for this specific > case is added in the series, but by logic in this patch summary > there's no reason to treat it differently. > > kernel/bpf/verifier.c | 15 ++------------- > 1 file changed, 2 insertions(+), 13 deletions(-) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 97351ae3e7a7..ddc1452cf023 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -5634,17 +5634,6 @@ struct bpf_reg_types { > u32 *btf_id; > }; > > -static const struct bpf_reg_types map_key_value_types = { > - .types = { > - PTR_TO_STACK, > - PTR_TO_PACKET, > - PTR_TO_PACKET_META, > - PTR_TO_MAP_KEY, > - PTR_TO_MAP_VALUE, > - PTR_TO_MEM | MEM_ALLOC, > - }, > -}; > - > static const struct bpf_reg_types sock_types = { > .types = { > PTR_TO_SOCK_COMMON, > @@ -5711,8 +5700,8 @@ static const struct bpf_reg_types dynptr_types = { > }; > > static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = { > - [ARG_PTR_TO_MAP_KEY] = &map_key_value_types, > - [ARG_PTR_TO_MAP_VALUE] = &map_key_value_types, > + [ARG_PTR_TO_MAP_KEY] = &mem_types, > + [ARG_PTR_TO_MAP_VALUE] = &mem_types, > [ARG_CONST_SIZE] = &scalar_types, > [ARG_CONST_SIZE_OR_ZERO] = &scalar_types, > [ARG_CONST_ALLOC_SIZE_OR_ZERO] = &scalar_types, > -- > 2.30.2 >
On Fri, Oct 21, 2022 at 4:04 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Thu, Oct 20, 2022 at 9:07 AM Dave Marchevsky <davemarchevsky@fb.com> wrote: > > > > After the previous patch, which added PTR_TO_MEM | MEM_ALLOC type > > map_key_value_types, the only difference between map_key_value_types and > > mem_types sets is PTR_TO_BUF and PTR_TO_MEM, which are in the latter set > > but not the former. > > > > Helpers which expect ARG_PTR_TO_MAP_KEY or ARG_PTR_TO_MAP_VALUE > > already effectively expect a valid blob of arbitrary memory that isn't > > necessarily explicitly associated with a map. When validating a > > PTR_TO_MAP_{KEY,VALUE} arg, the verifier expects meta->map_ptr to have > > already been set, either by an earlier ARG_CONST_MAP_PTR arg, or custom > > logic like that in process_timer_func or process_kptr_func. > > > > So let's get rid of map_key_value_types and just use mem_types for those > > args. > > > > This has the effect of adding PTR_TO_BUF and PTR_TO_MEM to the set of > > compatible types for ARG_PTR_TO_MAP_KEY and ARG_PTR_TO_MAP_VALUE. > > > > PTR_TO_BUF is used by various bpf_iter implementations to represent a > > chunk of valid r/w memory in ctx args for iter prog. > > > > PTR_TO_MEM is used by networking, tracing, and ringbuf helpers to > > represent a chunk of valid memory. The PTR_TO_MEM | MEM_ALLOC > > type added in previous commmit is specific to ringbuf helpers. > > typo: s/commmit/commit/ (but not worth reposting just to fix this) Patched it up while applying.
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 97351ae3e7a7..ddc1452cf023 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -5634,17 +5634,6 @@ struct bpf_reg_types { u32 *btf_id; }; -static const struct bpf_reg_types map_key_value_types = { - .types = { - PTR_TO_STACK, - PTR_TO_PACKET, - PTR_TO_PACKET_META, - PTR_TO_MAP_KEY, - PTR_TO_MAP_VALUE, - PTR_TO_MEM | MEM_ALLOC, - }, -}; - static const struct bpf_reg_types sock_types = { .types = { PTR_TO_SOCK_COMMON, @@ -5711,8 +5700,8 @@ static const struct bpf_reg_types dynptr_types = { }; static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = { - [ARG_PTR_TO_MAP_KEY] = &map_key_value_types, - [ARG_PTR_TO_MAP_VALUE] = &map_key_value_types, + [ARG_PTR_TO_MAP_KEY] = &mem_types, + [ARG_PTR_TO_MAP_VALUE] = &mem_types, [ARG_CONST_SIZE] = &scalar_types, [ARG_CONST_SIZE_OR_ZERO] = &scalar_types, [ARG_CONST_ALLOC_SIZE_OR_ZERO] = &scalar_types,
After the previous patch, which added PTR_TO_MEM | MEM_ALLOC type map_key_value_types, the only difference between map_key_value_types and mem_types sets is PTR_TO_BUF and PTR_TO_MEM, which are in the latter set but not the former. Helpers which expect ARG_PTR_TO_MAP_KEY or ARG_PTR_TO_MAP_VALUE already effectively expect a valid blob of arbitrary memory that isn't necessarily explicitly associated with a map. When validating a PTR_TO_MAP_{KEY,VALUE} arg, the verifier expects meta->map_ptr to have already been set, either by an earlier ARG_CONST_MAP_PTR arg, or custom logic like that in process_timer_func or process_kptr_func. So let's get rid of map_key_value_types and just use mem_types for those args. This has the effect of adding PTR_TO_BUF and PTR_TO_MEM to the set of compatible types for ARG_PTR_TO_MAP_KEY and ARG_PTR_TO_MAP_VALUE. PTR_TO_BUF is used by various bpf_iter implementations to represent a chunk of valid r/w memory in ctx args for iter prog. PTR_TO_MEM is used by networking, tracing, and ringbuf helpers to represent a chunk of valid memory. The PTR_TO_MEM | MEM_ALLOC type added in previous commmit is specific to ringbuf helpers. Presence or absence of MEM_ALLOC doesn't change the validity of using PTR_TO_MEM as a map_{key,val} input. Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com> --- v1 -> v5: lore.kernel.org/bpf/20220912101106.2765921-2-davemarchevsky@fb.com * This patch was dropped in v2 as I had no concrete usecase for PTR_TO_BUF and PTR_TO_MEM w/o MEM_ALLOC. Andrii encouraged me to re-add the patch as we both share desire to eventually cleanup all these separate "valid chunk of memory" types. Starting to treat them similarly is a good step in that direction. * A usecase for PTR_TO_BUF is now demonstrated in patch 4 of this series. * PTR_TO_MEM w/o MEM_ALLOC is returned by bpf_{this,per}_cpu_ptr helpers via RET_PTR_TO_MEM_OR_BTF_ID, but in both cases the return type is also tagged MEM_RDONLY, which map helpers don't currently accept (see patch 4 summary). So no selftest for this specific case is added in the series, but by logic in this patch summary there's no reason to treat it differently. kernel/bpf/verifier.c | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-)