Re: [PATCH bpf-next v1 3/7] bpf: Rework process_dynptr_func

[David Vernet <void@manifault.com>]

On Tue, Nov 15, 2022 at 05:31:26AM +0530, Kumar Kartikeya Dwivedi wrote:
> Recently, user ringbuf support introduced a PTR_TO_DYNPTR register type
> for use in callback state, because in case of user ringbuf helpers,
> there is no dynptr on the stack that is passed into the callback. To
> reflect such a state, a special register type was created.
> 
> However, some checks have been bypassed incorrectly during the addition
> of this feature. First, for arg_type with MEM_UNINIT flag which
> initialize a dynptr, they must be rejected for such register type.
> Secondly, in the future, there are plans to add dynptr helpers that
> operate on the dynptr itself and may change its offset and other
> properties.
> 
> In all of these cases, PTR_TO_DYNPTR shouldn't be allowed to be passed
> to such helpers, however the current code simply returns 0.
> 
> The rejection for helpers that release the dynptr is already handled.
> 
> For fixing this, we take a step back and rework existing code in a way
> that will allow fitting in all classes of helpers and have a coherent
> model for dealing with the variety of use cases in which dynptr is used.
> 
> First, for ARG_PTR_TO_DYNPTR, it can either be set alone or together
> with a DYNPTR_TYPE_* constant that denotes the only type it accepts.
> 
> Next, helpers which initialize a dynptr use MEM_UNINIT to indicate this
> fact. To make the distinction clear, use MEM_RDONLY flag to indicate
> that the helper only operates on the memory pointed to by the dynptr,
> not the dynptr itself. In C parlance, it would be equivalent to taking
> the dynptr as a point to const argument.
> 
> When either of these flags are not present, the helper is allowed to
> mutate both the dynptr itself and also the memory it points to.
> Currently, the read only status of the memory is not tracked in the
> dynptr, but it would be trivial to add this support inside dynptr state
> of the register.
> 
> With these changes and renaming PTR_TO_DYNPTR to CONST_PTR_TO_DYNPTR to
> better reflect its usage, it can no longer be passed to helpers that
> initialize a dynptr, i.e. bpf_dynptr_from_mem, bpf_ringbuf_reserve_dynptr.
> 
> A note to reviewers is that in code that does mark_stack_slots_dynptr,
> and unmark_stack_slots_dynptr, we implicitly rely on the fact that
> PTR_TO_STACK reg is the only case that can reach that code path, as one
> cannot pass CONST_PTR_TO_DYNPTR to helpers that don't set MEM_RDONLY. In
> both cases such helpers won't be setting that flag.
> 
> The next patch will add a couple of selftest cases to make sure this
> doesn't break.
> 
> Fixes: 205715673844 ("bpf: Add bpf_user_ringbuf_drain() helper")
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---

Overall this LGTM. Left some comments / nits before stamping.

>  include/linux/bpf.h                           |   4 +-
>  include/uapi/linux/bpf.h                      |   8 +-
>  kernel/bpf/btf.c                              |   7 +-
>  kernel/bpf/helpers.c                          |  18 +-
>  kernel/bpf/verifier.c                         | 220 +++++++++++++-----
>  scripts/bpf_doc.py                            |   1 +
>  tools/include/uapi/linux/bpf.h                |   8 +-
>  .../selftests/bpf/prog_tests/user_ringbuf.c   |  10 +-
>  8 files changed, 195 insertions(+), 81 deletions(-)
> 
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 798aec816970..dfe45f6caa4f 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -709,7 +709,7 @@ enum bpf_reg_type {
>  	PTR_TO_MEM,		 /* reg points to valid memory region */
>  	PTR_TO_BUF,		 /* reg points to a read/write buffer */
>  	PTR_TO_FUNC,		 /* reg points to a bpf program function */
> -	PTR_TO_DYNPTR,		 /* reg points to a dynptr */
> +	CONST_PTR_TO_DYNPTR,	 /* reg points to a const struct bpf_dynptr */
>  	__BPF_REG_TYPE_MAX,
>  
>  	/* Extended reg_types. */
> @@ -2761,7 +2761,7 @@ void bpf_dynptr_init(struct bpf_dynptr_kern *ptr, void *data,
>  		     enum bpf_dynptr_type type, u32 offset, u32 size);
>  void bpf_dynptr_set_null(struct bpf_dynptr_kern *ptr);
>  int bpf_dynptr_check_size(u32 size);
> -u32 bpf_dynptr_get_size(struct bpf_dynptr_kern *ptr);
> +u32 bpf_dynptr_get_size(const struct bpf_dynptr_kern *ptr);
>  
>  #ifdef CONFIG_BPF_LSM
>  void bpf_cgroup_atype_get(u32 attach_btf_id, int cgroup_atype);
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index fb4c911d2a03..b7543efd6284 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -5290,7 +5290,7 @@ union bpf_attr {
>   *	Return
>   *		Nothing. Always succeeds.
>   *
> - * long bpf_dynptr_read(void *dst, u32 len, struct bpf_dynptr *src, u32 offset, u64 flags)
> + * long bpf_dynptr_read(void *dst, u32 len, const struct bpf_dynptr *src, u32 offset, u64 flags)
>   *	Description
>   *		Read *len* bytes from *src* into *dst*, starting from *offset*
>   *		into *src*.
> @@ -5300,7 +5300,7 @@ union bpf_attr {
>   *		of *src*'s data, -EINVAL if *src* is an invalid dynptr or if
>   *		*flags* is not 0.
>   *
> - * long bpf_dynptr_write(struct bpf_dynptr *dst, u32 offset, void *src, u32 len, u64 flags)
> + * long bpf_dynptr_write(const struct bpf_dynptr *dst, u32 offset, void *src, u32 len, u64 flags)
>   *	Description
>   *		Write *len* bytes from *src* into *dst*, starting from *offset*
>   *		into *dst*.
> @@ -5310,7 +5310,7 @@ union bpf_attr {
>   *		of *dst*'s data, -EINVAL if *dst* is an invalid dynptr or if *dst*
>   *		is a read-only dynptr or if *flags* is not 0.
>   *
> - * void *bpf_dynptr_data(struct bpf_dynptr *ptr, u32 offset, u32 len)
> + * void *bpf_dynptr_data(const struct bpf_dynptr *ptr, u32 offset, u32 len)
>   *	Description
>   *		Get a pointer to the underlying dynptr data.
>   *
> @@ -5411,7 +5411,7 @@ union bpf_attr {
>   *		Drain samples from the specified user ring buffer, and invoke
>   *		the provided callback for each such sample:
>   *
> - *		long (\*callback_fn)(struct bpf_dynptr \*dynptr, void \*ctx);
> + *		long (\*callback_fn)(const struct bpf_dynptr \*dynptr, void \*ctx);
>   *
>   *		If **callback_fn** returns 0, the helper will continue to try
>   *		and drain the next sample, up to a maximum of
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index d02ae2f4249b..ba3b50895f6b 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -6568,14 +6568,15 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
>  				}
>  
>  				if (arg_dynptr) {
> -					if (reg->type != PTR_TO_STACK) {
> -						bpf_log(log, "arg#%d pointer type %s %s not to stack\n",
> +					if (reg->type != PTR_TO_STACK &&
> +					    reg->type != CONST_PTR_TO_DYNPTR) {
> +						bpf_log(log, "arg#%d pointer type %s %s not to stack or dynptr\n",
>  							i, btf_type_str(ref_t),
>  							ref_tname);
>  						return -EINVAL;
>  					}

Should we bring this check into process_dynptr_func()? It's kind of
unfortunate that we have divergent logic between helpers and kfuncs,
when it comes to checking types. Let's at least be uniform here?

> -					if (process_dynptr_func(env, regno, ARG_PTR_TO_DYNPTR, NULL))
> +					if (process_dynptr_func(env, regno, ARG_PTR_TO_DYNPTR | MEM_RDONLY, NULL))
>  						return -EINVAL;
>  					continue;
>  				}
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 283f55bbeb70..714f5c9d0c1f 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -1398,7 +1398,7 @@ static const struct bpf_func_proto bpf_kptr_xchg_proto = {
>  #define DYNPTR_SIZE_MASK	0xFFFFFF
>  #define DYNPTR_RDONLY_BIT	BIT(31)
>  
> -static bool bpf_dynptr_is_rdonly(struct bpf_dynptr_kern *ptr)
> +static bool bpf_dynptr_is_rdonly(const struct bpf_dynptr_kern *ptr)
>  {
>  	return ptr->size & DYNPTR_RDONLY_BIT;
>  }
> @@ -1408,7 +1408,7 @@ static void bpf_dynptr_set_type(struct bpf_dynptr_kern *ptr, enum bpf_dynptr_typ
>  	ptr->size |= type << DYNPTR_TYPE_SHIFT;
>  }
>  
> -u32 bpf_dynptr_get_size(struct bpf_dynptr_kern *ptr)
> +u32 bpf_dynptr_get_size(const struct bpf_dynptr_kern *ptr)
>  {
>  	return ptr->size & DYNPTR_SIZE_MASK;
>  }
> @@ -1432,7 +1432,7 @@ void bpf_dynptr_set_null(struct bpf_dynptr_kern *ptr)
>  	memset(ptr, 0, sizeof(*ptr));
>  }
>  
> -static int bpf_dynptr_check_off_len(struct bpf_dynptr_kern *ptr, u32 offset, u32 len)
> +static int bpf_dynptr_check_off_len(const struct bpf_dynptr_kern *ptr, u32 offset, u32 len)
>  {
>  	u32 size = bpf_dynptr_get_size(ptr);
>  
> @@ -1477,7 +1477,7 @@ static const struct bpf_func_proto bpf_dynptr_from_mem_proto = {
>  	.arg4_type	= ARG_PTR_TO_DYNPTR | DYNPTR_TYPE_LOCAL | MEM_UNINIT,
>  };
>  
> -BPF_CALL_5(bpf_dynptr_read, void *, dst, u32, len, struct bpf_dynptr_kern *, src,
> +BPF_CALL_5(bpf_dynptr_read, void *, dst, u32, len, const struct bpf_dynptr_kern *, src,
>  	   u32, offset, u64, flags)
>  {
>  	int err;
> @@ -1500,12 +1500,12 @@ static const struct bpf_func_proto bpf_dynptr_read_proto = {
>  	.ret_type	= RET_INTEGER,
>  	.arg1_type	= ARG_PTR_TO_UNINIT_MEM,
>  	.arg2_type	= ARG_CONST_SIZE_OR_ZERO,
> -	.arg3_type	= ARG_PTR_TO_DYNPTR,
> +	.arg3_type	= ARG_PTR_TO_DYNPTR | MEM_RDONLY,
>  	.arg4_type	= ARG_ANYTHING,
>  	.arg5_type	= ARG_ANYTHING,
>  };
>  
> -BPF_CALL_5(bpf_dynptr_write, struct bpf_dynptr_kern *, dst, u32, offset, void *, src,
> +BPF_CALL_5(bpf_dynptr_write, const struct bpf_dynptr_kern *, dst, u32, offset, void *, src,
>  	   u32, len, u64, flags)
>  {
>  	int err;
> @@ -1526,14 +1526,14 @@ static 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,
> +	.arg1_type	= ARG_PTR_TO_DYNPTR | MEM_RDONLY,
>  	.arg2_type	= ARG_ANYTHING,
>  	.arg3_type	= ARG_PTR_TO_MEM | MEM_RDONLY,
>  	.arg4_type	= ARG_CONST_SIZE_OR_ZERO,
>  	.arg5_type	= ARG_ANYTHING,
>  };
>  
> -BPF_CALL_3(bpf_dynptr_data, struct bpf_dynptr_kern *, ptr, u32, offset, u32, len)
> +BPF_CALL_3(bpf_dynptr_data, const struct bpf_dynptr_kern *, ptr, u32, offset, u32, len)
>  {
>  	int err;
>  
> @@ -1554,7 +1554,7 @@ static const struct bpf_func_proto bpf_dynptr_data_proto = {
>  	.func		= bpf_dynptr_data,
>  	.gpl_only	= false,
>  	.ret_type	= RET_PTR_TO_DYNPTR_MEM_OR_NULL,
> -	.arg1_type	= ARG_PTR_TO_DYNPTR,
> +	.arg1_type	= ARG_PTR_TO_DYNPTR | MEM_RDONLY,
>  	.arg2_type	= ARG_ANYTHING,
>  	.arg3_type	= ARG_CONST_ALLOC_SIZE_OR_ZERO,
>  };
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 41ef7e4b73e4..c484e632b0cd 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -565,7 +565,7 @@ static const char *reg_type_str(struct bpf_verifier_env *env,
>  		[PTR_TO_BUF]		= "buf",
>  		[PTR_TO_FUNC]		= "func",
>  		[PTR_TO_MAP_KEY]	= "map_key",
> -		[PTR_TO_DYNPTR]		= "dynptr_ptr",
> +		[CONST_PTR_TO_DYNPTR]	= "dynptr",
>  	};
>  
>  	if (type & PTR_MAYBE_NULL) {
> @@ -699,6 +699,27 @@ static bool dynptr_type_refcounted(enum bpf_dynptr_type type)
>  	return type == BPF_DYNPTR_TYPE_RINGBUF;
>  }
>  
> +static void __mark_dynptr_regs(struct bpf_reg_state *reg1,
> +			       struct bpf_reg_state *reg2,
> +			       enum bpf_dynptr_type type);
> +
> +static void __mark_reg_not_init(const struct bpf_verifier_env *env,
> +				struct bpf_reg_state *reg);
> +
> +static void mark_dynptr_stack_regs(struct bpf_reg_state *sreg1,
> +				   struct bpf_reg_state *sreg2,
> +				   enum bpf_dynptr_type type)
> +{
> +	__mark_dynptr_regs(sreg1, sreg2, type);
> +}
> +
> +static void mark_dynptr_cb_reg(struct bpf_reg_state *reg1,
> +			       enum bpf_dynptr_type type)
> +{
> +	__mark_dynptr_regs(reg1, NULL, type);
> +}
> +
> +
>  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)
>  {
> @@ -720,9 +741,8 @@ static int mark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_
>  	if (type == BPF_DYNPTR_TYPE_INVALID)
>  		return -EINVAL;
>  
> -	state->stack[spi].spilled_ptr.dynptr.first_slot = true;
> -	state->stack[spi].spilled_ptr.dynptr.type = type;
> -	state->stack[spi - 1].spilled_ptr.dynptr.type = type;
> +	mark_dynptr_stack_regs(&state->stack[spi].spilled_ptr,
> +			       &state->stack[spi - 1].spilled_ptr, type);
>  
>  	if (dynptr_type_refcounted(type)) {
>  		/* The id is used to track proper releasing */
> @@ -730,8 +750,8 @@ static int mark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_
>  		if (id < 0)
>  			return id;
>  
> -		state->stack[spi].spilled_ptr.id = id;
> -		state->stack[spi - 1].spilled_ptr.id = id;
> +		state->stack[spi].spilled_ptr.ref_obj_id = id;
> +		state->stack[spi - 1].spilled_ptr.ref_obj_id = id;
>  	}
>  
>  	return 0;
> @@ -753,25 +773,23 @@ static int unmark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_re
>  	}
>  
>  	/* Invalidate any slices associated with this dynptr */
> -	if (dynptr_type_refcounted(state->stack[spi].spilled_ptr.dynptr.type)) {
> -		release_reference(env, state->stack[spi].spilled_ptr.id);
> -		state->stack[spi].spilled_ptr.id = 0;
> -		state->stack[spi - 1].spilled_ptr.id = 0;
> -	}
> -
> -	state->stack[spi].spilled_ptr.dynptr.first_slot = false;
> -	state->stack[spi].spilled_ptr.dynptr.type = 0;
> -	state->stack[spi - 1].spilled_ptr.dynptr.type = 0;
> +	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);
>  	return 0;
>  }
>  
>  static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
>  {
>  	struct bpf_func_state *state = func(env, reg);
> -	int spi = get_spi(reg->off);
> -	int i;
> +	int spi, i;
> +
> +	if (reg->type == CONST_PTR_TO_DYNPTR)
> +		return false;
>  
> +	spi = get_spi(reg->off);
>  	if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS))
>  		return true;
>  
> @@ -787,9 +805,14 @@ static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_
>  static bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
>  {
>  	struct bpf_func_state *state = func(env, reg);
> -	int spi = get_spi(reg->off);
> +	int spi;
>  	int i;
>  
> +	/* This already represents first slot of initialized bpf_dynptr */
> +	if (reg->type == CONST_PTR_TO_DYNPTR)
> +		return true;
> +
> +	spi = get_spi(reg->off);
>  	if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS) ||
>  	    !state->stack[spi].spilled_ptr.dynptr.first_slot)
>  		return false;
> @@ -808,15 +831,19 @@ static bool is_dynptr_type_expected(struct bpf_verifier_env *env, struct bpf_reg
>  {
>  	struct bpf_func_state *state = func(env, reg);
>  	enum bpf_dynptr_type dynptr_type;
> -	int spi = get_spi(reg->off);
> +	int spi;
>  
>  	/* ARG_PTR_TO_DYNPTR takes any type of dynptr */
>  	if (arg_type == ARG_PTR_TO_DYNPTR)
>  		return true;
>  
>  	dynptr_type = arg_to_dynptr_type(arg_type);
> -
> -	return state->stack[spi].spilled_ptr.dynptr.type == dynptr_type;
> +	if (reg->type == CONST_PTR_TO_DYNPTR) {
> +		return reg->dynptr.type == dynptr_type;
> +	} else {
> +		spi = get_spi(reg->off);
> +		return state->stack[spi].spilled_ptr.dynptr.type == dynptr_type;
> +	}
>  }
>  
>  /* The reg state of a pointer or a bounded scalar was saved when
> @@ -1324,9 +1351,6 @@ static const int caller_saved[CALLER_SAVED_REGS] = {
>  	BPF_REG_0, BPF_REG_1, BPF_REG_2, BPF_REG_3, BPF_REG_4, BPF_REG_5
>  };
>  
> -static void __mark_reg_not_init(const struct bpf_verifier_env *env,
> -				struct bpf_reg_state *reg);
> -
>  /* This helper doesn't clear reg->id */
>  static void ___mark_reg_known(struct bpf_reg_state *reg, u64 imm)
>  {
> @@ -1389,6 +1413,26 @@ static void mark_reg_known_zero(struct bpf_verifier_env *env,
>  	__mark_reg_known_zero(regs + regno);
>  }
>  
> +static void __mark_dynptr_regs(struct bpf_reg_state *reg1,
> +			       struct bpf_reg_state *reg2,
> +			       enum bpf_dynptr_type type)
> +{
> +	/* reg->type has no meaning for STACK_DYNPTR, but when we set reg for
> +	 * callback arguments, it does need to be CONST_PTR_TO_DYNPTR, so simply
> +	 * set it unconditionally as it is ignored for STACK_DYNPTR anyway.
> +	 */
> +	__mark_reg_known_zero(reg1);
> +	reg1->type = CONST_PTR_TO_DYNPTR;
> +	reg1->dynptr.type = type;
> +	reg1->dynptr.first_slot = true;
> +	if (!reg2)
> +		return;
> +	__mark_reg_known_zero(reg2);
> +	reg2->type = CONST_PTR_TO_DYNPTR;
> +	reg2->dynptr.type = type;
> +	reg2->dynptr.first_slot = false;
> +}
> +
>  static void mark_ptr_not_null_reg(struct bpf_reg_state *reg)
>  {
>  	if (base_type(reg->type) == PTR_TO_MAP_VALUE) {
> @@ -5692,20 +5736,62 @@ static int process_kptr_func(struct bpf_verifier_env *env, int regno,
>  	return 0;
>  }
>  
> +/* Implementation details:

I don't think "Implementation details" is necessary to include in the
function header (and thank you for adding this header btw).

> + *
> + * There are two register types representing a bpf_dynptr, one is PTR_TO_STACK
> + * which points to a stack slot, and the other is CONST_PTR_TO_DYNPTR.
> + *
> + * In both cases we deal with the first 8 bytes, but need to mark the next 8
> + * bytes as STACK_DYNPTR in case of PTR_TO_STACK. In case of
> + * CONST_PTR_TO_DYNPTR, we are guaranteed to get the beginning of the object.
> + *
> + * Mutability of bpf_dynptr is at two levels, one is at the level of struct
> + * bpf_dynptr itself, i.e. whether the helper is receiving a pointer to struct
> + * bpf_dynptr or pointer to const struct bpf_dynptr. In the former case, it can
> + * mutate the view of the dynptr and also possibly destroy it. In the latter
> + * case, it cannot mutate the bpf_dynptr itself but it can still mutate the
> + * memory that dynptr points to.
> + *
> + * The verifier will keep track both levels of mutation (bpf_dynptr's in
> + * reg->type and the memory's in reg->dynptr.type), but there is no support for
> + * readonly dynptr view yet, hence only the first case is tracked and checked.
> + *
> + * This is consistent with how C applies the const modifier to a struct object,
> + * where the pointer itself inside bpf_dynptr becomes const but not what it
> + * points to.
> + *
> + * Helpers which do not mutate the bpf_dynptr set MEM_RDONLY in their argument
> + * type, and declare it as 'const struct bpf_dynptr *' in their prototype.
> + */
>  int process_dynptr_func(struct bpf_verifier_env *env, int regno,
> -			enum bpf_arg_type arg_type,
> -			struct bpf_call_arg_meta *meta)
> +			enum bpf_arg_type arg_type, struct bpf_call_arg_meta *meta)
>  {
>  	struct bpf_reg_state *regs = cur_regs(env), *reg = &regs[regno];
>  	int argno = regno - 1;
>  
> -	/* We only need to check for initialized / uninitialized helper
> -	 * dynptr args if the dynptr is not PTR_TO_DYNPTR, as the
> -	 * assumption is that if it is, that a helper function
> -	 * initialized the dynptr on behalf of the BPF program.
> +	if ((arg_type & (MEM_UNINIT | MEM_RDONLY)) == (MEM_UNINIT | MEM_RDONLY)) {
> +		verbose(env, "verifier internal error: misconfigured dynptr helper type flags\n");
> +		return -EFAULT;
> +	}
> +
> +	/* MEM_UNINIT and MEM_RDONLY are exclusive, when applied to a

s/applied to a/applied to an

> +	 * ARG_PTR_TO_DYNPTR (or ARG_PTR_TO_DYNPTR | DYNPTR_TYPE_*):
> +	 *
> +	 *  MEM_UNINIT - Points to memory that is an appropriate candidate for
> +	 *		 constructing a mutable bpf_dynptr object.
> +	 *
> +	 *		 Currently, this is only possible with PTR_TO_STACK
> +	 *		 pointing to a region of atleast 16 bytes which doesn't

s/atleast/at least

> +	 *		 contain an existing bpf_dynptr.
> +	 *
> +	 *  MEM_RDONLY - Points to a initialized bpf_dynptr that will not be
> +	 *		 mutated or destroyed. However, the memory it points to
> +	 *		 may be mutated.
> +	 *
> +	 *  None       - Points to a initialized dynptr that can be mutated and
> +	 *		 destroyed, including mutation of the memory it points
> +	 *		 to.
>  	 */
> -	if (base_type(reg->type) == PTR_TO_DYNPTR)
> -		return 0;
>  	if (arg_type & MEM_UNINIT) {
>  		if (!is_dynptr_reg_valid_uninit(env, reg)) {
>  			verbose(env, "Dynptr has to be an uninitialized dynptr\n");

Not your change, but this is an awkwardly phrased error message. IMO
"dynptr must be initialized" is more succinct. Feel free to ignore if
you'd like, I'm happy to submit a separate patch to change it as some
point.

> @@ -5722,6 +5808,12 @@ int process_dynptr_func(struct bpf_verifier_env *env, int regno,
>  
>  		meta->uninit_dynptr_regno = regno;
>  	} else {
> +		/* For the reg->type == PTR_TO_STACK case, bpf_dynptr is never const */
> +		if (reg->type == CONST_PTR_TO_DYNPTR && !(arg_type & MEM_RDONLY)) {
> +			verbose(env, "cannot pass pointer to const bpf_dynptr, the helper mutates it\n");
> +			return -EINVAL;
> +		}
> +
>  		if (!is_dynptr_reg_valid_init(env, reg)) {
>  			verbose(env,
>  				"Expected an initialized dynptr as arg #%d\n",
> @@ -5729,6 +5821,8 @@ int process_dynptr_func(struct bpf_verifier_env *env, int regno,
>  			return -EINVAL;
>  		}
>  
> +		/* Fold MEM_RDONLY, only inspect arg_type */
> +		arg_type &= ~MEM_RDONLY;

This seems brittle. Can is_dynptr_type_expected() just check the base
type?

>  		if (!is_dynptr_type_expected(env, reg, arg_type)) {
>  			const char *err_extra = "";
>  
> @@ -5874,7 +5968,7 @@ static const struct bpf_reg_types kptr_types = { .types = { PTR_TO_MAP_VALUE } }
>  static const struct bpf_reg_types dynptr_types = {
>  	.types = {
>  		PTR_TO_STACK,
> -		PTR_TO_DYNPTR | DYNPTR_TYPE_LOCAL,
> +		CONST_PTR_TO_DYNPTR,
>  	}
>  };
>  
> @@ -6050,12 +6144,15 @@ int check_func_arg_reg_off(struct bpf_verifier_env *env,
>  	return __check_ptr_off_reg(env, reg, regno, fixed_off_ok);
>  }
>  
> -static u32 stack_slot_get_id(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
> +static u32 dynptr_ref_obj_id(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
>  {
>  	struct bpf_func_state *state = func(env, reg);
> -	int spi = get_spi(reg->off);
> +	int spi;
>  
> -	return state->stack[spi].spilled_ptr.id;
> +	if (reg->type == CONST_PTR_TO_DYNPTR)
> +		return reg->ref_obj_id;
> +	spi = get_spi(reg->off);
> +	return state->stack[spi].spilled_ptr.ref_obj_id;
>  }
>  
>  static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> @@ -6119,11 +6216,17 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
>  	if (arg_type_is_release(arg_type)) {
>  		if (arg_type_is_dynptr(arg_type)) {
>  			struct bpf_func_state *state = func(env, reg);
> -			int spi = get_spi(reg->off);
> +			int spi;
>  
> -			if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS) ||
> -			    !state->stack[spi].spilled_ptr.id) {
> -				verbose(env, "arg %d is an unacquired reference\n", regno);

Can we add a comment here explaining why only PTR_TO_STACK dynptrs are
expected to be released? I know we have such comments elsewhere already,
but if we're going to have logic like this which is hard-coded against
assumptions of what types of dynptrs can be used in which contexts /
helpers, I think it is important to be verbose in calling that out as
it's not obvious from the code itself why this is the case.

> +			if (reg->type == PTR_TO_STACK) {
> +				spi = get_spi(reg->off);
> +				if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS) ||
> +				    !state->stack[spi].spilled_ptr.ref_obj_id) {
> +					verbose(env, "arg %d is an unacquired reference\n", regno);
> +					return -EINVAL;
> +				}
> +			} else {
> +				verbose(env, "cannot release unowned const bpf_dynptr\n");
>  				return -EINVAL;
>  			}
>  		} else if (!reg->ref_obj_id && !register_is_null(reg)) {
> @@ -7091,11 +7194,10 @@ static int set_user_ringbuf_callback_state(struct bpf_verifier_env *env,
>  {
>  	/* bpf_user_ringbuf_drain(struct bpf_map *map, void *callback_fn, void
>  	 *			  callback_ctx, u64 flags);
> -	 * callback_fn(struct bpf_dynptr_t* dynptr, void *callback_ctx);
> +	 * callback_fn(const struct bpf_dynptr_t* dynptr, void *callback_ctx);
>  	 */
>  	__mark_reg_not_init(env, &callee->regs[BPF_REG_0]);
> -	callee->regs[BPF_REG_1].type = PTR_TO_DYNPTR | DYNPTR_TYPE_LOCAL;
> -	__mark_reg_known_zero(&callee->regs[BPF_REG_1]);
> +	mark_dynptr_cb_reg(&callee->regs[BPF_REG_1], BPF_DYNPTR_TYPE_LOCAL);
>  	callee->regs[BPF_REG_2] = caller->regs[BPF_REG_3];
>  
>  	/* unused */
> @@ -7474,7 +7576,13 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
>  
>  	regs = cur_regs(env);
>  
> +	/* This can only be set for PTR_TO_STACK, as CONST_PTR_TO_DYNPTR cannot
> +	 * be reinitialized by any dynptr helper. Hence, mark_stack_slots_dynptr
> +	 * is safe to do directly.
> +	 */
>  	if (meta.uninit_dynptr_regno) {
> +		if (WARN_ON_ONCE(regs[meta.uninit_dynptr_regno].type == CONST_PTR_TO_DYNPTR))

I think we tend to do "verifier internal error" logs for situations like
this rather than WARN_ON_ONCE(), right?

> +			return -EFAULT;
>  		/* we write BPF_DW bits (8 bytes) at a time */
>  		for (i = 0; i < BPF_DYNPTR_SIZE; i += 8) {
>  			err = check_mem_access(env, insn_idx, meta.uninit_dynptr_regno,
> @@ -7492,15 +7600,22 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
>  
>  	if (meta.release_regno) {
>  		err = -EINVAL;
> -		if (arg_type_is_dynptr(fn->arg_type[meta.release_regno - BPF_REG_1]))
> +		/* This can only be set for PTR_TO_STACK, as CONST_PTR_TO_DYNPTR cannot
> +		 * be released by any dynptr helper. Hence, unmark_stack_slots_dynptr
> +		 * is safe to do directly.
> +		 */
> +		if (arg_type_is_dynptr(fn->arg_type[meta.release_regno - BPF_REG_1])) {
> +			if (WARN_ON_ONCE(regs[meta.release_regno].type == CONST_PTR_TO_DYNPTR))

Same here r.e. using verifier log rather than WARN_ON_ONCE(). Also, can
we bring this comment inside the if statement to match the alignemnt of
the one you added below?

> +				return -EFAULT;
>  			err = unmark_stack_slots_dynptr(env, &regs[meta.release_regno]);
> -		else if (meta.ref_obj_id)
> +		} else if (meta.ref_obj_id) {
>  			err = release_reference(env, meta.ref_obj_id);
> -		/* meta.ref_obj_id can only be 0 if register that is meant to be
> -		 * released is NULL, which must be > R0.
> -		 */
> -		else if (register_is_null(&regs[meta.release_regno]))
> +		} else if (register_is_null(&regs[meta.release_regno])) {
> +			/* meta.ref_obj_id can only be 0 if register that is meant to be
> +			 * released is NULL, which must be > R0.
> +			 */
>  			err = 0;
> +		}
>  		if (err) {
>  			verbose(env, "func %s#%d reference has not been acquired before\n",
>  				func_id_name(func_id), func_id);
> @@ -7574,11 +7689,8 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
>  					return -EFAULT;
>  				}
>  
> -				if (base_type(reg->type) != PTR_TO_DYNPTR)
> -					/* Find the id of the dynptr we're
> -					 * tracking the reference of
> -					 */
> -					meta.ref_obj_id = stack_slot_get_id(env, reg);
> +				/* Find the id of the dynptr we're tracking the reference of */

Not your change, but IMO this comment does not add any value.

> +				meta.ref_obj_id = dynptr_ref_obj_id(env, reg);
>  				break;
>  			}
>  		}
> diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py
> index fdb0aff8cb5a..e8d90829f23e 100755
> --- a/scripts/bpf_doc.py
> +++ b/scripts/bpf_doc.py
> @@ -752,6 +752,7 @@ class PrinterHelpers(Printer):
>              'struct bpf_timer',
>              'struct mptcp_sock',
>              'struct bpf_dynptr',
> +            'const struct bpf_dynptr',
>              'struct iphdr',
>              'struct ipv6hdr',
>      }
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index fb4c911d2a03..b7543efd6284 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -5290,7 +5290,7 @@ union bpf_attr {
>   *	Return
>   *		Nothing. Always succeeds.
>   *
> - * long bpf_dynptr_read(void *dst, u32 len, struct bpf_dynptr *src, u32 offset, u64 flags)
> + * long bpf_dynptr_read(void *dst, u32 len, const struct bpf_dynptr *src, u32 offset, u64 flags)
>   *	Description
>   *		Read *len* bytes from *src* into *dst*, starting from *offset*
>   *		into *src*.
> @@ -5300,7 +5300,7 @@ union bpf_attr {
>   *		of *src*'s data, -EINVAL if *src* is an invalid dynptr or if
>   *		*flags* is not 0.
>   *
> - * long bpf_dynptr_write(struct bpf_dynptr *dst, u32 offset, void *src, u32 len, u64 flags)
> + * long bpf_dynptr_write(const struct bpf_dynptr *dst, u32 offset, void *src, u32 len, u64 flags)
>   *	Description
>   *		Write *len* bytes from *src* into *dst*, starting from *offset*
>   *		into *dst*.
> @@ -5310,7 +5310,7 @@ union bpf_attr {
>   *		of *dst*'s data, -EINVAL if *dst* is an invalid dynptr or if *dst*
>   *		is a read-only dynptr or if *flags* is not 0.
>   *
> - * void *bpf_dynptr_data(struct bpf_dynptr *ptr, u32 offset, u32 len)
> + * void *bpf_dynptr_data(const struct bpf_dynptr *ptr, u32 offset, u32 len)
>   *	Description
>   *		Get a pointer to the underlying dynptr data.
>   *
> @@ -5411,7 +5411,7 @@ union bpf_attr {
>   *		Drain samples from the specified user ring buffer, and invoke
>   *		the provided callback for each such sample:
>   *
> - *		long (\*callback_fn)(struct bpf_dynptr \*dynptr, void \*ctx);
> + *		long (\*callback_fn)(const struct bpf_dynptr \*dynptr, void \*ctx);
>   *
>   *		If **callback_fn** returns 0, the helper will continue to try
>   *		and drain the next sample, up to a maximum of
> diff --git a/tools/testing/selftests/bpf/prog_tests/user_ringbuf.c b/tools/testing/selftests/bpf/prog_tests/user_ringbuf.c
> index 02b18d018b36..39882580cb90 100644
> --- a/tools/testing/selftests/bpf/prog_tests/user_ringbuf.c
> +++ b/tools/testing/selftests/bpf/prog_tests/user_ringbuf.c
> @@ -668,13 +668,13 @@ static struct {
>  	const char *expected_err_msg;
>  } failure_tests[] = {
>  	/* failure cases */
> -	{"user_ringbuf_callback_bad_access1", "negative offset dynptr_ptr ptr"},
> -	{"user_ringbuf_callback_bad_access2", "dereference of modified dynptr_ptr ptr"},
> -	{"user_ringbuf_callback_write_forbidden", "invalid mem access 'dynptr_ptr'"},
> +	{"user_ringbuf_callback_bad_access1", "negative offset dynptr ptr"},
> +	{"user_ringbuf_callback_bad_access2", "dereference of modified dynptr ptr"},
> +	{"user_ringbuf_callback_write_forbidden", "invalid mem access 'dynptr'"},
>  	{"user_ringbuf_callback_null_context_write", "invalid mem access 'scalar'"},
>  	{"user_ringbuf_callback_null_context_read", "invalid mem access 'scalar'"},
> -	{"user_ringbuf_callback_discard_dynptr", "arg 1 is an unacquired reference"},
> -	{"user_ringbuf_callback_submit_dynptr", "arg 1 is an unacquired reference"},
> +	{"user_ringbuf_callback_discard_dynptr", "cannot release unowned const bpf_dynptr"},
> +	{"user_ringbuf_callback_submit_dynptr", "cannot release unowned const bpf_dynptr"},
>  	{"user_ringbuf_callback_invalid_return", "At callback return the register R0 has value"},
>  };
>  
> -- 
> 2.38.1
>