On Mon, May 16, 2022 at 9:56 AM David Vernet <void@xxxxxxxxxxxxx> wrote:
>
> On Mon, May 09, 2022 at 03:42:55PM -0700, Joanne Koong wrote:
> > 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 | 16 ++++++++++
> > include/uapi/linux/bpf.h | 19 ++++++++++++
> > kernel/bpf/helpers.c | 56 ++++++++++++++++++++++++++++++++++
> > tools/include/uapi/linux/bpf.h | 19 ++++++++++++
> > 4 files changed, 110 insertions(+)
> >
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index 8fbe739b0dec..6f4fa0627620 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -2391,6 +2391,12 @@ enum bpf_dynptr_type {
> > #define DYNPTR_SIZE_MASK 0xFFFFFF
> > #define DYNPTR_TYPE_SHIFT 28
> > #define DYNPTR_TYPE_MASK 0x7
> > +#define DYNPTR_RDONLY_BIT BIT(31)
> > +
> > +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)
> > {
> > @@ -2412,6 +2418,16 @@ static inline int bpf_dynptr_check_size(u32 size)
> > return size > DYNPTR_MAX_SIZE ? -E2BIG : 0;
> > }
> >
> > +static inline int bpf_dynptr_check_off_len(struct bpf_dynptr_kern *ptr, u32 offset, u32 len)
> > +{
> > + u32 size = bpf_dynptr_get_size(ptr);
> > +
> > + if (len > size || offset > size - len)
> > + return -E2BIG;
> > +
> > + return 0;
> > +}
>
> Does this need to be in bpf.h? Or could it be brought into helpers.c as a
> static function? I don't think there's any harm in leaving it here, but at
> first glance it seems like a helper function that doesn't really need to be
> exported.
I will move this function back to helpers.c
>
> > +
> > void bpf_dynptr_init(struct bpf_dynptr_kern *ptr, void *data, enum bpf_dynptr_type type,
> > u32 offset, u32 size);
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 679f960d2514..f0c5ca220d8e 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -5209,6 +5209,23 @@ union bpf_attr {
> > * 'bpf_ringbuf_discard'.
> > * Return
> > * Nothing. Always succeeds.
> > + *
> > + * 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 *src*.
> > + * Return
> > + * 0 on success, -E2BIG if *offset* + *len* exceeds the length
> > + * of *src*'s data, -EINVAL 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, -E2BIG if *offset* + *len* exceeds the length
> > + * of *dst*'s data, -EINVAL if *dst* is an invalid dynptr or if *dst*
> > + * is a read-only dynptr.
> > */
> > #define __BPF_FUNC_MAPPER(FN) \
> > FN(unspec), \
> > @@ -5411,6 +5428,8 @@ union bpf_attr {
> > FN(ringbuf_reserve_dynptr), \
> > FN(ringbuf_submit_dynptr), \
> > FN(ringbuf_discard_dynptr), \
> > + 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 2d6f2e28b580..7206b9e5322f 100644
> > --- a/kernel/bpf/helpers.c
> > +++ b/kernel/bpf/helpers.c
> > @@ -1467,6 +1467,58 @@ const struct bpf_func_proto bpf_dynptr_put_proto = {
> > .arg1_type = ARG_PTR_TO_DYNPTR | DYNPTR_TYPE_MALLOC | OBJ_RELEASE,
> > };
> >
> > +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);
> > + if (err)
> > + return err;
> > +
> > + memcpy(dst, src->data + src->offset + offset, len);
> > +
> > + return 0;
> > +}
> > +
> > +const struct bpf_func_proto bpf_dynptr_read_proto = {
> > + .func = bpf_dynptr_read,
> > + .gpl_only = false,
> > + .ret_type = RET_INTEGER,
> > + .arg1_type = ARG_PTR_TO_UNINIT_MEM,
> > + .arg2_type = ARG_CONST_SIZE_OR_ZERO,
> > + .arg3_type = ARG_PTR_TO_DYNPTR,
> > + .arg4_type = ARG_ANYTHING,
>
> I think what you have now is safe / correct, but is there a reason that we
> don't use ARG_CONST_SIZE_OR_ZERO for both the len and the offset, given
> that they're both bound by the size of a memory region? Same question
> applies to the function proto for bpf_dynptr_write() as well.
I think it offers more flexibility as an API if the offset doesn't
have to be statically known (eg the program can use an offset that is
set by the userspace application).
>
> > +};
> > +
> > +BPF_CALL_4(bpf_dynptr_write, struct bpf_dynptr_kern *, dst, u32, offset, void *, src, u32, len)
> > +{
> > + int err;
> > +
> > + if (!dst->data || bpf_dynptr_is_rdonly(dst))
> > + return -EINVAL;
> > +
> > + err = bpf_dynptr_check_off_len(dst, offset, len);
> > + if (err)
> > + return err;
> > +
> > + memcpy(dst->data + dst->offset + offset, src, len);
> > +
> > + return 0;
> > +}
> > +
> > +const struct bpf_func_proto bpf_dynptr_write_proto = {
> > + .func = bpf_dynptr_write,
> > + .gpl_only = false,
> > + .ret_type = RET_INTEGER,
> > + .arg1_type = ARG_PTR_TO_DYNPTR,
> > + .arg2_type = ARG_ANYTHING,
> > + .arg3_type = ARG_PTR_TO_MEM | MEM_RDONLY,
> > + .arg4_type = ARG_CONST_SIZE_OR_ZERO,
> > +};
> > +
>
> [...]
>
> Overall looks great.
