Re: [PATCH bpf-next v5 05/25] bpf: Rename MEM_ALLOC to MEM_RINGBUF

From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Cc: bpf@vger.kernel.org, Alexei Starovoitov <ast@kernel.org>,
	Andrii Nakryiko <andrii@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Martin KaFai Lau <martin.lau@kernel.org>,
	Dave Marchevsky <davemarchevsky@meta.com>,
	Delyan Kratunov <delyank@meta.com>
Subject: Re: [PATCH bpf-next v5 05/25] bpf: Rename MEM_ALLOC to MEM_RINGBUF
Date: Tue, 8 Nov 2022 15:14:16 -0800	[thread overview]
Message-ID: <CAEf4BzZ180YJ+fbynJSR2fXXMVuKZTyginHyRdxydvOm-po7TA@mail.gmail.com> (raw)
In-Reply-To: <20221107230950.7117-6-memxor@gmail.com>

On Mon, Nov 7, 2022 at 3:10 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
>
> Currently, verifier uses MEM_ALLOC type tag to specially tag memory
> returned from bpf_ringbuf_reserve helper. However, this is currently
> only used for this purpose and there is an implicit assumption that it
> only refers to ringbuf memory (e.g. the check for ARG_PTR_TO_ALLOC_MEM
> in check_func_arg_reg_off).
>
> Hence, rename MEM_ALLOC to MEM_RINGBUF to indicate this special
> relationship and instead open the use of MEM_ALLOC for more generic
> allocations made for user types.
>
> Also, since ARG_PTR_TO_ALLOC_MEM_OR_NULL is unused, simply drop it.
>
> Finally, update selftests using 'alloc_' verifier string to 'ringbuf_'.
>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---

Ok, so you are doing what I asked in the previous patch, nice :)

>  include/linux/bpf.h                               | 11 ++++-------
>  kernel/bpf/ringbuf.c                              |  6 +++---
>  kernel/bpf/verifier.c                             | 14 +++++++-------
>  tools/testing/selftests/bpf/prog_tests/dynptr.c   |  2 +-
>  tools/testing/selftests/bpf/verifier/ringbuf.c    |  2 +-
>  tools/testing/selftests/bpf/verifier/spill_fill.c |  2 +-
>  6 files changed, 17 insertions(+), 20 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 2fe3ec620d54..afc1c51b59ff 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -488,10 +488,8 @@ enum bpf_type_flag {
>          */
>         MEM_RDONLY              = BIT(1 + BPF_BASE_TYPE_BITS),
>
> -       /* MEM was "allocated" from a different helper, and cannot be mixed
> -        * with regular non-MEM_ALLOC'ed MEM types.
> -        */
> -       MEM_ALLOC               = BIT(2 + BPF_BASE_TYPE_BITS),
> +       /* MEM points to BPF ring buffer reservation. */
> +       MEM_RINGBUF             = BIT(2 + BPF_BASE_TYPE_BITS),

What do we gain by having ringbuf memory as additional modified flag
instead of its own type like PTR_TO_MAP_VALUE or PTR_TO_PACKET? It
feels like here separate register type is more justified and is less
error prone overall.

>
>         /* MEM is in user address space. */
>         MEM_USER                = BIT(3 + BPF_BASE_TYPE_BITS),
> @@ -565,7 +563,7 @@ enum bpf_arg_type {
>         ARG_PTR_TO_LONG,        /* pointer to long */
>         ARG_PTR_TO_SOCKET,      /* pointer to bpf_sock (fullsock) */
>         ARG_PTR_TO_BTF_ID,      /* pointer to in-kernel struct */
> -       ARG_PTR_TO_ALLOC_MEM,   /* pointer to dynamically allocated memory */
> +       ARG_PTR_TO_RINGBUF_MEM, /* pointer to dynamically reserved ringbuf memory */
>         ARG_CONST_ALLOC_SIZE_OR_ZERO,   /* number of allocated bytes requested */
>         ARG_PTR_TO_BTF_ID_SOCK_COMMON,  /* pointer to in-kernel sock_common or bpf-mirrored bpf_sock */
>         ARG_PTR_TO_PERCPU_BTF_ID,       /* pointer to in-kernel percpu type */

[...]