Re: [PATCH bpf-next v1 03/13] bpf: Rename confusingly named RET_PTR_TO_ALLOC_MEM

From: sdf@google.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>,
	Joanne Koong <joannelkoong@gmail.com>,
	David Vernet <void@manifault.com>
Subject: Re: [PATCH bpf-next v1 03/13] bpf: Rename confusingly named RET_PTR_TO_ALLOC_MEM
Date: Tue, 18 Oct 2022 14:38:21 -0700	[thread overview]
Message-ID: <Y08czdCQgMig/Wir@google.com> (raw)
In-Reply-To: <20221018135920.726360-4-memxor@gmail.com>

On 10/18, Kumar Kartikeya Dwivedi 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@gmail.com>
> ---
>   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 13c6ff2de540..834276ba56c9 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -538,7 +538,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 dynamically allocated memory  
> */

What about the comment? It still says that it's a pointer to a
dynamically allocated memory :-/ Does it make sense to clarify it as
well?

>   	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,
> @@ -548,8 +548,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 87d9cccd1623..a49b95c1af1b 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -7612,7 +7612,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.0