On Thu, Apr 20, 2023 at 12:15 AM Joanne Koong <joannelkoong@xxxxxxxxx> wrote:
>
> 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>
> ---
> kernel/bpf/helpers.c | 14 ++++++
> kernel/bpf/verifier.c | 105 ++++++++++++++++++++++++++++++++++--------
> 2 files changed, 99 insertions(+), 20 deletions(-)
>
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 9018646b86db..1ebdc7f1a574 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -2343,6 +2343,19 @@ __bpf_kfunc __u32 bpf_dynptr_size(const struct bpf_dynptr_kern *ptr)
> return __bpf_dynptr_size(ptr);
> }
>
> +__bpf_kfunc int bpf_dynptr_clone(struct bpf_dynptr_kern *ptr,
> + struct bpf_dynptr_kern *clone__uninit)
> +{
> + if (!ptr->data) {
> + bpf_dynptr_set_null(clone__uninit);
> + return -EINVAL;
> + }
> +
> + *clone__uninit = *ptr;
> +
> + return 0;
> +}
> +
> __bpf_kfunc void *bpf_cast_to_kern_ctx(void *obj)
> {
> return obj;
> @@ -2419,6 +2432,7 @@ BTF_ID_FLAGS(func, bpf_dynptr_adjust)
> BTF_ID_FLAGS(func, bpf_dynptr_is_null)
> BTF_ID_FLAGS(func, bpf_dynptr_is_rdonly)
> BTF_ID_FLAGS(func, bpf_dynptr_size)
> +BTF_ID_FLAGS(func, bpf_dynptr_clone)
> BTF_SET8_END(common_btf_ids)
>
> static const struct btf_kfunc_id_set common_kfunc_set = {
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 1e05355facdc..164726673086 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -309,6 +309,7 @@ struct bpf_kfunc_call_arg_meta {
> struct {
> enum bpf_dynptr_type type;
> u32 id;
> + u32 ref_obj_id;
> } initialized_dynptr;
> struct {
> u8 spi;
> @@ -847,11 +848,11 @@ static int destroy_if_dynptr_stack_slot(struct bpf_verifier_env *env,
> struct bpf_func_state *state, int spi);
>
> 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)
> + enum bpf_arg_type arg_type, int insn_idx, int clone_ref_obj_id)
> {
> struct bpf_func_state *state = func(env, reg);
> enum bpf_dynptr_type type;
> - int spi, i, id, err;
> + int spi, i, err;
>
> spi = dynptr_get_spi(env, reg);
> if (spi < 0)
> @@ -887,7 +888,13 @@ static int mark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_
>
> if (dynptr_type_refcounted(type)) {
> /* The id is used to track proper releasing */
> - id = acquire_reference_state(env, insn_idx);
> + int id;
> +
> + if (clone_ref_obj_id)
> + id = clone_ref_obj_id;
> + else
> + id = acquire_reference_state(env, insn_idx);
> +
> if (id < 0)
> return id;
>
> @@ -901,24 +908,15 @@ static int mark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_
> return 0;
> }
>
> -static int unmark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
> +static void invalidate_dynptr(struct bpf_verifier_env *env, struct bpf_func_state *state, int spi)
> {
> - struct bpf_func_state *state = func(env, reg);
> - int spi, i;
> -
> - spi = dynptr_get_spi(env, reg);
> - if (spi < 0)
> - return spi;
> + int i;
>
> for (i = 0; i < BPF_REG_SIZE; i++) {
> state->stack[spi].slot_type[i] = STACK_INVALID;
> state->stack[spi - 1].slot_type[i] = STACK_INVALID;
> }
>
> - /* Invalidate any slices associated with this dynptr */
> - if (dynptr_type_refcounted(state->stack[spi].spilled_ptr.dynptr.type))
> - WARN_ON_ONCE(release_reference(env, state->stack[spi].spilled_ptr.ref_obj_id));
> -
> __mark_reg_not_init(env, &state->stack[spi].spilled_ptr);
> __mark_reg_not_init(env, &state->stack[spi - 1].spilled_ptr);
>
> @@ -945,6 +943,52 @@ static int unmark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_re
> */
> state->stack[spi].spilled_ptr.live |= REG_LIVE_WRITTEN;
> state->stack[spi - 1].spilled_ptr.live |= REG_LIVE_WRITTEN;
> +}
> +
> +static int unmark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
> +{
> + struct bpf_func_state *state = func(env, reg);
> + int spi;
> +
> + spi = dynptr_get_spi(env, reg);
> + if (spi < 0)
> + return spi;
> +
> + if (dynptr_type_refcounted(state->stack[spi].spilled_ptr.dynptr.type)) {
I inverted this condition, did invalidate_dynptr(spi), return 0. This
reduced nestedness of refcounted case handling code below.
The rest looks great!
> + int ref_obj_id = state->stack[spi].spilled_ptr.ref_obj_id;
> + int i;
> +
> + /* If the dynptr has a ref_obj_id, then we need to invalidate
> + * two things:
> + *
> + * 1) Any dynptrs with a matching ref_obj_id (clones)
> + * 2) Any slices derived from this dynptr.
> + */
> +
[...]
