On Mon, Nov 7, 2022 at 3:10 PM Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx> wrote:
>
> Currently, the verifier has two return types, RET_PTR_TO_ALLOC_MEM, and
> RET_PTR_TO_ALLOC_MEM_OR_NULL, however the former is confusingly named to
> imply that it carries MEM_ALLOC, while only the latter does. This causes
> confusion during code review leading to conclusions like that the return
> value of RET_PTR_TO_DYNPTR_MEM_OR_NULL (which is RET_PTR_TO_ALLOC_MEM |
> PTR_MAYBE_NULL) may be consumable by bpf_ringbuf_{submit,commit}.
>
> Rename it to make it clear MEM_ALLOC needs to be tacked on top of
> RET_PTR_TO_MEM.
>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> ---
The whole MEM_ALLOC as related to ringbuf is so confusing. Why can't
be just call it for what it is: RET_PTR_TO_RINGBUF_MEM_OR_NULL,
ARG_PTR_TO_RINGBUF_MEM, PTR_TO_RINGBUF_MEM ?
It would be also much easier to make sure (by looking at the code)
that ringbuf invariants are properly checked.
> include/linux/bpf.h | 6 +++---
> kernel/bpf/verifier.c | 2 +-
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 05f98e9e5c48..2fe3ec620d54 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -607,7 +607,7 @@ enum bpf_return_type {
> RET_PTR_TO_SOCKET, /* returns a pointer to a socket */
> RET_PTR_TO_TCP_SOCK, /* returns a pointer to a tcp_sock */
> RET_PTR_TO_SOCK_COMMON, /* returns a pointer to a sock_common */
> - RET_PTR_TO_ALLOC_MEM, /* returns a pointer to dynamically allocated memory */
> + RET_PTR_TO_MEM, /* returns a pointer to memory */
> RET_PTR_TO_MEM_OR_BTF_ID, /* returns a pointer to a valid memory or a btf_id */
> RET_PTR_TO_BTF_ID, /* returns a pointer to a btf_id */
> __BPF_RET_TYPE_MAX,
> @@ -617,8 +617,8 @@ enum bpf_return_type {
> RET_PTR_TO_SOCKET_OR_NULL = PTR_MAYBE_NULL | RET_PTR_TO_SOCKET,
> RET_PTR_TO_TCP_SOCK_OR_NULL = PTR_MAYBE_NULL | RET_PTR_TO_TCP_SOCK,
> RET_PTR_TO_SOCK_COMMON_OR_NULL = PTR_MAYBE_NULL | RET_PTR_TO_SOCK_COMMON,
> - RET_PTR_TO_ALLOC_MEM_OR_NULL = PTR_MAYBE_NULL | MEM_ALLOC | RET_PTR_TO_ALLOC_MEM,
> - RET_PTR_TO_DYNPTR_MEM_OR_NULL = PTR_MAYBE_NULL | RET_PTR_TO_ALLOC_MEM,
> + RET_PTR_TO_ALLOC_MEM_OR_NULL = PTR_MAYBE_NULL | MEM_ALLOC | RET_PTR_TO_MEM,
> + RET_PTR_TO_DYNPTR_MEM_OR_NULL = PTR_MAYBE_NULL | RET_PTR_TO_MEM,
> RET_PTR_TO_BTF_ID_OR_NULL = PTR_MAYBE_NULL | RET_PTR_TO_BTF_ID,
>
> /* This must be the last entry. Its purpose is to ensure the enum is
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 0374f03d1f56..2407e3b179ec 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -7621,7 +7621,7 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
> mark_reg_known_zero(env, regs, BPF_REG_0);
> regs[BPF_REG_0].type = PTR_TO_TCP_SOCK | ret_flag;
> break;
> - case RET_PTR_TO_ALLOC_MEM:
> + case RET_PTR_TO_MEM:
> mark_reg_known_zero(env, regs, BPF_REG_0);
> regs[BPF_REG_0].type = PTR_TO_MEM | ret_flag;
> regs[BPF_REG_0].mem_size = meta.mem_size;
> --
> 2.38.1
>
