diff mbox series

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

Message ID 20221018135920.726360-4-memxor@gmail.com (mailing list archive)
State Changes Requested
Delegated to: BPF
Headers show
Series Fixes for dynptr | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1360 this patch: 1360
netdev/cc_maintainers warning 8 maintainers not CCed: sdf@google.com john.fastabend@gmail.com yhs@fb.com haoluo@google.com jolsa@kernel.org kpsingh@kernel.org song@kernel.org martin.lau@linux.dev
netdev/build_clang success Errors and warnings before: 157 this patch: 157
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1350 this patch: 1350
netdev/checkpatch warning WARNING: line length of 86 exceeds 80 columns WARNING: line length of 95 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-PR fail PR summary
bpf/vmtest-bpf-next-VM_Test-4 success Logs for llvm-toolchain
bpf/vmtest-bpf-next-VM_Test-5 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-2 success Logs for build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-3 success Logs for build for x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-1 success Logs for build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-7 success Logs for test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for test_maps on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-13 fail Logs for test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-14 fail Logs for test_progs_no_alu32 on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-16 success Logs for test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for test_verifier on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-10 fail Logs for test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-11 fail Logs for test_progs on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-15 success Logs for test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-9 fail Logs for test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-12 fail Logs for test_progs_no_alu32 on s390x with gcc

Commit Message

Kumar Kartikeya Dwivedi Oct. 18, 2022, 1:59 p.m. UTC 
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(-)

Comments

Stanislav Fomichev Oct. 18, 2022, 9:38 p.m. UTC | #1 
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
Kumar Kartikeya Dwivedi Oct. 19, 2022, 6:19 a.m. UTC | #2 
On Wed, Oct 19, 2022 at 03:08:21AM IST, sdf@google.com wrote:
> 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?
>

Argh, right, I will change that. Thanks for spotting it!
Joanne Koong Nov. 7, 2022, 10:35 p.m. UTC | #3 
On Tue, Oct 18, 2022 at 6:59 AM Kumar Kartikeya Dwivedi
<memxor@gmail.com> 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 */
>         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,

Can you also rename this to RET_PTR_TO_RINGBUF_MEM_OR_NULL instead of
RET_PTR_TO_ALLOC_MEM_OR_NULL, and MEM_RINGBUF instead of MEM_ALLOC?
RET_PTR_TO_ALLOC_MEM_OR_NULL only pertains to ringbuf records, not
generic dynamically allocated memory, so I think this rename would
make this a lot more clear.

> +       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
>
Kumar Kartikeya Dwivedi Nov. 7, 2022, 11:12 p.m. UTC | #4 
On Tue, Nov 08, 2022 at 04:05:22AM IST, Joanne Koong wrote:
> On Tue, Oct 18, 2022 at 6:59 AM Kumar Kartikeya Dwivedi
> <memxor@gmail.com> 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 */
> >         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,
>
> Can you also rename this to RET_PTR_TO_RINGBUF_MEM_OR_NULL instead of
> RET_PTR_TO_ALLOC_MEM_OR_NULL, and MEM_RINGBUF instead of MEM_ALLOC?
> RET_PTR_TO_ALLOC_MEM_OR_NULL only pertains to ringbuf records, not
> generic dynamically allocated memory, so I think this rename would
> make this a lot more clear.
>

I have posted it here: https://lore.kernel.org/bpf/20221107230950.7117-6-memxor@gmail.com
diff mbox series

Patch

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 */
 	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;