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.
>
> > +
> > __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?
>
> > + 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;
> > + }
>
> [...]
