On Wed, Sep 7, 2022 at 5:07 PM Joanne Koong <joannelkoong@xxxxxxxxx> wrote:
>
> Add a new helper, bpf_dynptr_clone, which clones a dynptr.
>
> The cloned dynptr will point to the same data as its parent dynptr,
> with the same type, offset, size and read-only properties.
>
> Any writes to a dynptr will be reflected across all instances
> (by 'instance', this means any dynptrs that point to the same
> underlying data).
>
> Please note that data slice and dynptr invalidations will affect all
> instances as well. For example, if bpf_dynptr_write() is called on an
> skb-type dynptr, all data slices of dynptr instances to that skb
> will be invalidated as well (eg data slices of any clones, parents,
> grandparents, ...). Another example is if a ringbuf dynptr is submitted,
> any instance of that dynptr will be invalidated.
>
> Changing the view of the dynptr (eg advancing the offset or
> trimming the size) will only affect that dynptr and not affect any
> other instances.
>
> One example use case where cloning may be helpful is for hashing or
> iterating through dynptr data. Cloning will allow the user to maintain
> the original view of the dynptr for future use, while also allowing
> views to smaller subsets of the data after the offset is advanced or the
> size is trimmed.
>
> Signed-off-by: Joanne Koong <joannelkoong@xxxxxxxxx>
> ---
> include/uapi/linux/bpf.h | 24 +++++++
> kernel/bpf/helpers.c | 23 +++++++
> kernel/bpf/verifier.c | 116 +++++++++++++++++++++------------
> tools/include/uapi/linux/bpf.h | 24 +++++++
> 4 files changed, 147 insertions(+), 40 deletions(-)
>
This is a very important helper in practice, looking forward to have
it as part of dynptr API family!
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 4ca07cf500d2..16973fa4d073 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -5508,6 +5508,29 @@ union bpf_attr {
> * Return
> * The offset of the dynptr on success, -EINVAL if the dynptr is
> * invalid.
> + *
> + * long bpf_dynptr_clone(struct bpf_dynptr *ptr, struct bpf_dynptr *clone)
const struct bpf_dynptr *ptr to make it clear that ptr is not
modified. We can also use src and dst terminology here for less
ambiguity.
> + * Description
> + * Clone an initialized dynptr *ptr*. After this call, both *ptr*
> + * and *clone* will point to the same underlying data.
> + *
> + * *clone* must be an uninitialized dynptr.
> + *
> + * Any data slice or dynptr invalidations will apply equally for
> + * both dynptrs after this call. For example, if ptr1 is a
> + * ringbuf-type dynptr with multiple data slices that is cloned to
> + * ptr2, if ptr2 discards the ringbuf sample, then ptr2, ptr2's
> + * data slices, ptr1, and ptr1's data slices will all be
> + * invalidated.
> + *
> + * This is convenient for getting different "views" to the same
> + * data. For instance, if one wishes to hash only a particular
> + * section of data, one can clone the dynptr, advance it to a
> + * specified offset and trim it to a specified size, pass it
> + * to the hash function, and discard it after hashing, without
> + * losing access to the original view of the dynptr.
> + * Return
> + * 0 on success, -EINVAL if the dynptr to clone is invalid.
> */
> #define __BPF_FUNC_MAPPER(FN) \
> FN(unspec), \
> @@ -5728,6 +5751,7 @@ union bpf_attr {
> FN(dynptr_is_rdonly), \
> FN(dynptr_get_size), \
> FN(dynptr_get_offset), \
> + FN(dynptr_clone), \
> /* */
>
> /* 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 62ed27444b73..667f1e213a61 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -1762,6 +1762,27 @@ static const struct bpf_func_proto bpf_dynptr_get_offset_proto = {
> .arg1_type = ARG_PTR_TO_DYNPTR,
> };
>
> +BPF_CALL_2(bpf_dynptr_clone, struct bpf_dynptr_kern *, ptr,
> + struct bpf_dynptr_kern *, clone)
> +{
> + if (!ptr->data) {
> + bpf_dynptr_set_null(clone);
> + return -EINVAL;
> + }
once we have MALLOC dynptr this will break without appropriate refcnt
bump. Let's have an explicit switch over all types of DYNPTR and error
out on "unknown" (really, forgotten) types of dynptr. Better safe than
having to debug corruptions in production.
> +
> + memcpy(clone, ptr, sizeof(*clone));
*clone = *ptr?
> +
> + return 0;
> +}
> +
> +static const struct bpf_func_proto bpf_dynptr_clone_proto = {
> + .func = bpf_dynptr_clone,
> + .gpl_only = false,
> + .ret_type = RET_INTEGER,
> + .arg1_type = ARG_PTR_TO_DYNPTR,
> + .arg2_type = ARG_PTR_TO_DYNPTR | MEM_UNINIT,
> +};
> +
> const struct bpf_func_proto bpf_get_current_task_proto __weak;
> const struct bpf_func_proto bpf_get_current_task_btf_proto __weak;
> const struct bpf_func_proto bpf_probe_read_user_proto __weak;
> @@ -1846,6 +1867,8 @@ bpf_base_func_proto(enum bpf_func_id func_id)
> return &bpf_dynptr_get_size_proto;
> case BPF_FUNC_dynptr_get_offset:
> return &bpf_dynptr_get_offset_proto;
> + case BPF_FUNC_dynptr_clone:
> + return &bpf_dynptr_clone_proto;
> default:
> break;
> }
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index c312d931359d..8c8c101513f5 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -694,17 +694,38 @@ static enum bpf_dynptr_type arg_to_dynptr_type(enum bpf_arg_type arg_type)
> }
> }
>
> +static bool arg_type_is_dynptr(enum bpf_arg_type type)
> +{
> + return base_type(type) == ARG_PTR_TO_DYNPTR;
> +}
> +
> static bool dynptr_type_refcounted(enum bpf_dynptr_type type)
just noticed this name, I think it's quite confusing and will get even
more so with MALLOC dynptr. It's not really reference *counted*, more
like verifier *tracks references*, right? Consider renaming this to
avoid this confusion. MALLOC dynptr will be both reference tracked
(internally for verifier) and reference counted (at runtime to keep it
alive).
> {
> return type == BPF_DYNPTR_TYPE_RINGBUF;
> }
>
> -static int mark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
> - enum bpf_arg_type arg_type, int insn_idx)
> +static enum bpf_dynptr_type stack_slot_get_dynptr_info(struct bpf_verifier_env *env,
> + struct bpf_reg_state *reg,
> + int *ref_obj_id)
> {
> struct bpf_func_state *state = func(env, reg);
> - enum bpf_dynptr_type type;
> - int spi, i, id;
> + int spi = get_spi(reg->off);
> +
> + if (ref_obj_id)
> + *ref_obj_id = state->stack[spi].spilled_ptr.id;
> +
> + return state->stack[spi].spilled_ptr.dynptr.type;
> +}
> +
> +static int mark_stack_slots_dynptr(struct bpf_verifier_env *env,
> + const struct bpf_func_proto *fn,
> + struct bpf_reg_state *reg,
> + enum bpf_arg_type arg_type,
> + int insn_idx, int func_id)
> +{
> + enum bpf_dynptr_type type = BPF_DYNPTR_TYPE_INVALID;
> + struct bpf_func_state *state = func(env, reg);
> + int spi, i, id = 0;
>
> spi = get_spi(reg->off);
>
> @@ -716,7 +737,24 @@ static int mark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_
> state->stack[spi - 1].slot_type[i] = STACK_DYNPTR;
> }
>
> - type = arg_to_dynptr_type(arg_type);
> + if (func_id == BPF_FUNC_dynptr_clone) {
> + /* find the type and id of the dynptr we're cloning and
> + * assign it to the clone
> + */
> + for (i = 0; i < MAX_BPF_FUNC_REG_ARGS; i++) {
> + enum bpf_arg_type t = fn->arg_type[i];
> +
> + if (arg_type_is_dynptr(t) && !(t & MEM_UNINIT)) {
> + type = stack_slot_get_dynptr_info(env,
> + &state->regs[BPF_REG_1 + i],
so given we hard-coded BPF_FUNC_dynptr_clone func ID, we know that we
need first argument, so no need for this loop?
> + &id);
> + break;
> + }
> + }
> + } else {
> + type = arg_to_dynptr_type(arg_type);
> + }
> +
> if (type == BPF_DYNPTR_TYPE_INVALID)
> return -EINVAL;
>
[...]
