On Mon, May 9, 2022 at 10:22 AM Joanne Koong <joannelkoong@xxxxxxxxx> wrote:
>
> On Fri, May 6, 2022 at 4:57 PM Andrii Nakryiko
> <andrii.nakryiko@xxxxxxxxx> wrote:
> >
> > On Thu, Apr 28, 2022 at 2:12 PM Joanne Koong <joannelkoong@xxxxxxxxx> wrote:
> > >
> > > This patch adds a new helper function
> > >
> > > void *bpf_dynptr_data(struct bpf_dynptr *ptr, u32 offset, u32 len);
> > >
> > > which returns a pointer to the underlying data of a dynptr. *len*
> > > must be a statically known value. The bpf program may access the returned
> > > data slice as a normal buffer (eg can do direct reads and writes), since
> > > the verifier associates the length with the returned pointer, and
> > > enforces that no out of bounds accesses occur.
> > >
> > > Signed-off-by: Joanne Koong <joannelkoong@xxxxxxxxx>
> > > ---
> > > include/linux/bpf.h | 4 +++
> > > include/uapi/linux/bpf.h | 12 +++++++
> > > kernel/bpf/helpers.c | 28 +++++++++++++++
> > > kernel/bpf/verifier.c | 64 ++++++++++++++++++++++++++++++----
> > > tools/include/uapi/linux/bpf.h | 12 +++++++
> > > 5 files changed, 114 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > > index b276dbf942dd..4d2de868bdbc 100644
> > > --- a/include/linux/bpf.h
> > > +++ b/include/linux/bpf.h
> > > @@ -397,6 +397,9 @@ enum bpf_type_flag {
> > > /* DYNPTR points to a ringbuf record. */
> > > DYNPTR_TYPE_RINGBUF = BIT(9 + BPF_BASE_TYPE_BITS),
> > >
> > > + /* MEM is memory owned by a dynptr */
> > > + MEM_DYNPTR = BIT(10 + BPF_BASE_TYPE_BITS),
> >
> > do we need this yet another bit? It seems like it only matters for
> > verifier log dynptr_ output? Can we just reuse MEM_ALLOC? Or there is
> > some ringbuf-specific logic that we'll interfere with? If feels a bit
> > unnecessary, let's think if we can avoid adding bits just for this.
> I think we do need this bit to differentiate between MEM_ALLOC and
> MEM_DYNPTR because otherwise, you would be able to pass in a dynptr
> data slice to the ringbuf bpf_ringbuf_submit/discard helpers.
Right :( I forgot and missed ARG_PTR_TO_ALLOC_MEM, which relies on
this. No big deal.
As an aside, I regret using super-generic ALLOC_MEM naming for
strictly ringbuf-specific register, RINGBUF_MEM or something would be
better. But we can improve that separately.
> >
> > > +
> > > __BPF_TYPE_LAST_FLAG = DYNPTR_TYPE_RINGBUF,
> > > };
> > >
> >
> > [...]
> >
> > > + if (is_dynptr_ref_function(func_id)) {
> > > + int i;
> > > +
> > > + /* Find the id of the dynptr we're acquiring a reference to */
> > > + for (i = 0; i < MAX_BPF_FUNC_REG_ARGS; i++) {
> > > + if (arg_type_is_dynptr(fn->arg_type[i])) {
> > > + id = stack_slot_get_id(env, ®s[BPF_REG_1 + i]);
> >
> > let's make sure that we have only one such argument?
> Are we able to assume it's guaranteed given that
> is_dynptr_ref_function() refers to BPF_FUNC_dynptr_data and the
> definition of bpf_dynptr_data will always only have one dynptr arg?
That's why I think we should add this check, to guarantee it. Your
code assumes the first dynptr argument is the only one, completely
ignoring any more potential dynptr arguments if they are there. I
don't think that would be correct if we ever have such helpers with
two dynptrs, so we should be explicit about preventing that.
> >
> > > + break;
> > > + }
> > > + }
> > > + if (unlikely(i == MAX_BPF_FUNC_REG_ARGS)) {
> >
> > please don't use unlikely(), especially for non-performance critical code path
> >
> > > + verbose(env, "verifier internal error: no dynptr args to a dynptr ref function");
> > > + return -EFAULT;
> > > + }
> > > + } else {
> > > + id = acquire_reference_state(env, insn_idx);
> > > + if (id < 0)
> > > + return id;
> > > + }
> >
> > [...]
