Re: [PATCH bpf-next v5 05/25] bpf: Rename MEM_ALLOC to MEM_RINGBUF

From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Kumar Kartikeya Dwivedi <memxor@gmail.com>,
	bpf <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>,
	Dave Marchevsky <davemarchevsky@meta.com>,
	Delyan Kratunov <delyank@meta.com>
Subject: Re: [PATCH bpf-next v5 05/25] bpf: Rename MEM_ALLOC to MEM_RINGBUF
Date: Wed, 9 Nov 2022 14:58:12 -0800	[thread overview]
Message-ID: <CAEf4BzZ2rtdx2WVOzojCWA8-qPdTe1kCQXQvk5Lg7LypJ21SXQ@mail.gmail.com> (raw)
In-Reply-To: <CAADnVQJnHLu30fxj3rpzNNMDseJDk2Rs37e9PrqLQ3n=UEtZcQ@mail.gmail.com>

On Tue, Nov 8, 2022 at 5:05 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Nov 8, 2022 at 4:26 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Tue, Nov 8, 2022 at 3:49 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> > >
> > > On Wed, Nov 09, 2022 at 04:44:16AM IST, Andrii Nakryiko wrote:
> > > > On Mon, Nov 7, 2022 at 3:10 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> > > > >
> > > > > Currently, verifier uses MEM_ALLOC type tag to specially tag memory
> > > > > returned from bpf_ringbuf_reserve helper. However, this is currently
> > > > > only used for this purpose and there is an implicit assumption that it
> > > > > only refers to ringbuf memory (e.g. the check for ARG_PTR_TO_ALLOC_MEM
> > > > > in check_func_arg_reg_off).
> > > > >
> > > > > Hence, rename MEM_ALLOC to MEM_RINGBUF to indicate this special
> > > > > relationship and instead open the use of MEM_ALLOC for more generic
> > > > > allocations made for user types.
> > > > >
> > > > > Also, since ARG_PTR_TO_ALLOC_MEM_OR_NULL is unused, simply drop it.
> > > > >
> > > > > Finally, update selftests using 'alloc_' verifier string to 'ringbuf_'.
> > > > >
> > > > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > > > > ---
> > > >
> > > > Ok, so you are doing what I asked in the previous patch, nice :)
> > > >
> > > > >  include/linux/bpf.h                               | 11 ++++-------
> > > > >  kernel/bpf/ringbuf.c                              |  6 +++---
> > > > >  kernel/bpf/verifier.c                             | 14 +++++++-------
> > > > >  tools/testing/selftests/bpf/prog_tests/dynptr.c   |  2 +-
> > > > >  tools/testing/selftests/bpf/verifier/ringbuf.c    |  2 +-
> > > > >  tools/testing/selftests/bpf/verifier/spill_fill.c |  2 +-
> > > > >  6 files changed, 17 insertions(+), 20 deletions(-)
> > > > >
> > > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > > > > index 2fe3ec620d54..afc1c51b59ff 100644
> > > > > --- a/include/linux/bpf.h
> > > > > +++ b/include/linux/bpf.h
> > > > > @@ -488,10 +488,8 @@ enum bpf_type_flag {
> > > > >          */
> > > > >         MEM_RDONLY              = BIT(1 + BPF_BASE_TYPE_BITS),
> > > > >
> > > > > -       /* MEM was "allocated" from a different helper, and cannot be mixed
> > > > > -        * with regular non-MEM_ALLOC'ed MEM types.
> > > > > -        */
> > > > > -       MEM_ALLOC               = BIT(2 + BPF_BASE_TYPE_BITS),
> > > > > +       /* MEM points to BPF ring buffer reservation. */
> > > > > +       MEM_RINGBUF             = BIT(2 + BPF_BASE_TYPE_BITS),
> > > >
> > > > What do we gain by having ringbuf memory as additional modified flag
> > > > instead of its own type like PTR_TO_MAP_VALUE or PTR_TO_PACKET? It
> > > > feels like here separate register type is more justified and is less
> > > > error prone overall.
> > > >
> > >
> > > I'm not sure it's all that different. It only matters when checking argument
> > > during release. We want to ensure it came from ringbuf_reserve. That's all,
> > > apart from that it's no different from PTR_TO_MEM. In all other places it's
> > > folded and code for PTR_TO_MEM is used. Same idea as PTR_TO_BTF_ID | MEM_ALLOC.
> > >
> > > But I don't feel too strongly, so if you still think it's better I can make the
> > > switch.
> >
> > Not strongly, but I think having this as a flag is more error prone.
> > For cases where ringbuf memory should be treated just as memory, we
> > should use the same mechanism we have for MAP_VALUE. But I haven't
> > checked how we deal with MAP_VALUE, if that's a special case
> > everywhere, in addition to generic PTR_TO_MEM, then fine. But if
> > having PTR_TO_RINGBUF_MEM is converted to PTR_TO_MEM generically where
> > needed, I'd have dedicated PTR_TO_RINGBUF_MEM.
>
> I don't think we can or at least it's not as easy to generalize
> ringbuf mem as map_value.
> iirc MEM_ALLOC was there to make sure reserver->commit is the same mem.
> That's what MEM_RINGBUF will doing after this patch.

I'm not sure we are talking about the same thing. My only point was to
have RINGBUF_MEM as *base type* instead of as an *add-on flag*.
MAP_VALUE was an example of another special register type that is base
type but behaves like PTR_TO_MEM for helpers that don't care about
where specifically memory is coming from. I didn't mean to unify
RINGBUF_MEM and MAP_VALUE in any way (if that's what you are
proposing, I'm actually not sure exactly).

But as I said, no big deal with a flag, we can always change that in
the future as well.