Message ID | 20221018135920.726360-4-memxor@gmail.com (mailing list archive) |
---|---|
State | Changes Requested |
Delegated to: | BPF |
Headers | show |
Series | Fixes for dynptr | expand |
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
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!
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 >
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 --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;
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(-)