| Message ID | 20221107230950.7117-5-memxor@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | Local kptrs, BPF linked lists | expand |
On Mon, Nov 7, 2022 at 3:10 PM 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> > --- 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 >
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;
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(-)