On Wed, Apr 6, 2022 at 3:32 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Fri, Apr 1, 2022 at 7:00 PM Joanne Koong <joannekoong@xxxxxx> wrote:
> >
> > From: Joanne Koong <joannelkoong@xxxxxxxxx>
> >
> > This patch adds two helper functions, bpf_dynptr_read and
> > bpf_dynptr_write:
> >
> > long bpf_dynptr_read(void *dst, u32 len, struct bpf_dynptr *src, u32 offset);
> >
> > long bpf_dynptr_write(struct bpf_dynptr *dst, u32 offset, void *src, u32 len);
> >
> > The dynptr passed into these functions must be valid dynptrs that have
> > been initialized.
> >
> > Signed-off-by: Joanne Koong <joannelkoong@xxxxxxxxx>
> > ---
> > include/linux/bpf.h | 6 ++++
> > include/uapi/linux/bpf.h | 18 +++++++++++
> > kernel/bpf/helpers.c | 56 ++++++++++++++++++++++++++++++++++
> > tools/include/uapi/linux/bpf.h | 18 +++++++++++
> > 4 files changed, 98 insertions(+)
> >
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index e0fcff9f2aee..cded9753fb7f 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -2426,6 +2426,12 @@ enum bpf_dynptr_type {
> > #define DYNPTR_MAX_SIZE ((1UL << 28) - 1)
> > #define DYNPTR_SIZE_MASK 0xFFFFFFF
> > #define DYNPTR_TYPE_SHIFT 29
> > +#define DYNPTR_RDONLY_BIT BIT(28)
> > +
> > +static inline bool bpf_dynptr_is_rdonly(struct bpf_dynptr_kern *ptr)
> > +{
> > + return ptr->size & DYNPTR_RDONLY_BIT;
> > +}
> >
> > static inline enum bpf_dynptr_type bpf_dynptr_get_type(struct bpf_dynptr_kern *ptr)
> > {
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 6a57d8a1b882..16a35e46be90 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -5175,6 +5175,22 @@ union bpf_attr {
> > * After this operation, *ptr* will be an invalidated dynptr.
> > * Return
> > * Void.
> > + *
> > + * long bpf_dynptr_read(void *dst, u32 len, struct bpf_dynptr *src, u32 offset)
> > + * Description
> > + * Read *len* bytes from *src* into *dst*, starting from *offset*
> > + * into *dst*.
> > + * Return
> > + * 0 on success, -EINVAL if *offset* + *len* exceeds the length
> > + * of *src*'s data or if *src* is an invalid dynptr.
> > + *
> > + * long bpf_dynptr_write(struct bpf_dynptr *dst, u32 offset, void *src, u32 len)
> > + * Description
> > + * Write *len* bytes from *src* into *dst*, starting from *offset*
> > + * into *dst*.
> > + * Return
> > + * 0 on success, -EINVAL if *offset* + *len* exceeds the length
> > + * of *dst*'s data or if *dst* is not writeable.
>
> Did you plan to also add a helper to copy from one dynptr to another?
> Something like
>
> long bpf_dynptr_copy(struct bpf_dynptr *dst, struct bpf_dyn_ptr *src, u32 len) ?
>
> Otherwise there won't be any way to copy memory from malloc'ed range
> to ringbuf, for example, without doing intermediate copy. Not sure
> what to do about extra offsets...
Yes! I plan for the 3rd patchset in this dynptr series to be around
convenience helpers, which will include bpf_dynptr_copy.
For the offsets, I was thinking just copy from src data + src internal
offset to dst data + dst internal offset, where there will also be
dynptr helper functions that can be called to adjust offsets
>
> > */
> > #define __BPF_FUNC_MAPPER(FN) \
> > FN(unspec), \
> > @@ -5374,6 +5390,8 @@ union bpf_attr {
> > FN(dynptr_from_mem), \
> > FN(malloc), \
> > FN(free), \
> > + FN(dynptr_read), \
> > + FN(dynptr_write), \
> > /* */
> >
> > /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > index ed5a7d9d0a18..7ec20e79928e 100644
> > --- a/kernel/bpf/helpers.c
> > +++ b/kernel/bpf/helpers.c
> > @@ -1412,6 +1412,58 @@ const struct bpf_func_proto bpf_dynptr_from_mem_proto = {
> > .arg3_type = ARG_PTR_TO_DYNPTR | DYNPTR_TYPE_LOCAL | MEM_UNINIT,
> > };
> >
> > +BPF_CALL_4(bpf_dynptr_read, void *, dst, u32, len, struct bpf_dynptr_kern *, src, u32, offset)
> > +{
> > + int err;
> > +
> > + if (!src->data)
> > + return -EINVAL;
> > +
> > + err = bpf_dynptr_check_off_len(src, offset, len);
>
> you defined this function in patch #3, but didn't use it there. Let's
> move the definition into this patch?
Sounds great!
>
> > + if (err)
> > + return err;
> > +
> > + memcpy(dst, src->data + src->offset + offset, len);
> > +
> > + return 0;
> > +}
> > +
>
> [...]
