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

[Kumar Kartikeya Dwivedi <memxor@gmail.com>]

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