On Thu, Oct 20, 2022 at 9:07 AM Dave Marchevsky <davemarchevsky@xxxxxx> 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@xxxxxx>
> ---
> v1 -> v5: lore.kernel.org/bpf/20220912101106.2765921-2-davemarchevsky@xxxxxx
>
> * 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@xxxxxxxxxx>
> * 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
>
