On Tue, Dec 19, 2023 at 6:19 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Thu, Dec 14, 2023 at 5:14 PM Andrii Nakryiko <andrii@xxxxxxxxxx> wrote:
> >
> > + } else if (arg->arg_type == (ARG_PTR_TO_DYNPTR | MEM_RDONLY)) {
> > + ret = process_dynptr_func(env, regno, -1, arg->arg_type, 0);
>
> Minor nit:
>
> It's a rdonly dynptr, but still... may be pass env->insn_idx instead of -1 ?
I don't mind, but you are right, it's MEM_RDONLY, so insn_idx
*shouldn't* be necessary, which is what I sort of tried to ensure
explicitly. But I think the real problem here is that perhaps
process_dynptr_func() should be split into two: one that initializes
dynptr slots (MEM_UNINIT case), and separate that validates that
dynptr is properly initialized on the stack (non-UNINIT).
I will try to remember to come back to this after the holidays and
clean it up, ok?
>
> Separately, I'm not sure why we still pass insn_idx in so many
> functions. Looks like we can use env->insn_idx pretty much everywhere
> and remove that argument.
Do not know :) But some of those insn_idx passing goes really deep. I
think there is a constant opportunity (and, really, the need) to keep
refactoring and simplifying things in verifier's logic.
>
> The first 5 patches are very tricky, but all tests are green,
> so they must be correct :)
fingers crossed... :)
