On Fri, Apr 1, 2022 at 7:00 PM Joanne Koong <joannekoong@xxxxxx> wrote:
>
> From: Joanne Koong <joannelkoong@xxxxxxxxx>
>
> Currently, our only way of writing dynamically-sized data into a ring
> buffer is through bpf_ringbuf_output but this incurs an extra memcpy
> cost. bpf_ringbuf_reserve + bpf_ringbuf_commit avoids this extra
> memcpy, but it can only safely support reservation sizes that are
> statically known since the verifier cannot guarantee that the bpf
> program won’t access memory outside the reserved space.
>
> The bpf_dynptr abstraction allows for dynamically-sized ring buffer
> reservations without the extra memcpy.
>
> There are 3 new APIs:
>
> long bpf_ringbuf_reserve_dynptr(void *ringbuf, u32 size, u64 flags, struct bpf_dynptr *ptr);
> void bpf_ringbuf_submit_dynptr(struct bpf_dynptr *ptr, u64 flags);
> void bpf_ringbuf_discard_dynptr(struct bpf_dynptr *ptr, u64 flags);
>
> These closely follow the functionalities of the original ringbuf APIs.
> For example, all ringbuffer dynptrs that have been reserved must be
> either submitted or discarded before the program exits.
>
> Signed-off-by: Joanne Koong <joannelkoong@xxxxxxxxx>
> ---
> include/linux/bpf.h | 10 ++++-
> include/uapi/linux/bpf.h | 30 ++++++++++++++
> kernel/bpf/helpers.c | 6 +++
> kernel/bpf/ringbuf.c | 71 ++++++++++++++++++++++++++++++++++
> kernel/bpf/verifier.c | 17 ++++++--
> tools/include/uapi/linux/bpf.h | 30 ++++++++++++++
> 6 files changed, 160 insertions(+), 4 deletions(-)
>
Looks great and is a very straightforward implementation, great job!
Please fix the warning and maybe expand a bit on "failure modes", but
otherwise:
Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> /* The upper 4 bits of dynptr->size are reserved. Consequently, the
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index c835e437cb28..778de0b052c1 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -5202,6 +5202,33 @@ union bpf_attr {
> * Pointer to the underlying dynptr data, NULL if the ptr is
> * read-only, if the dynptr is invalid, or if the offset and length
> * is out of bounds.
> + *
> + * long bpf_ringbuf_reserve_dynptr(void *ringbuf, u32 size, u64 flags, struct bpf_dynptr *ptr)
looking at this, out param dynptr at the end makes more sense again...
ok, I'm fine either way I guess :)
> + * Description
> + * Reserve *size* bytes of payload in a ring buffer *ringbuf*
> + * through the dynptr interface. *flags* must be 0.
> + * Return
> + * 0 on success, or a negative error in case of failure.
It would be good to mention that the verifier will enforce submit or
discard even when reservation fails. And submit_dnyptr/discard_dynptr
is a no-op for such failed/null dynptrs.
> + *
> + * void bpf_ringbuf_submit_dynptr(struct bpf_dynptr *ptr, u64 flags)
> + * Description
> + * Submit reserved ring buffer sample, pointed to by *data*,
> + * through the dynptr interface.
> + *
> + * For more information on *flags*, please see
> + * 'bpf_ringbuf_submit'.
> + * Return
> + * Nothing. Always succeeds.
> + *
[...]
