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@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
>
> 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.
