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

[David Vernet <void@manifault.com>]

On Sun, Nov 20, 2022 at 11:36:51PM +0530, Kumar Kartikeya Dwivedi wrote:

[...]

> > 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.
> >
> 
> Feel free to, since I think unrelated changes should not be mixed in this patch.

No problem, will do.

[...]

> > >  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.
> >
> 
> Sure, but you mean a code comment, right?

Yep, a code comment.

[...]