On Fri, 16 Feb 2024 at 12:23, Eduard Zingerman <eddyz87@xxxxxxxxx> wrote: > > On Thu, 2024-02-15 at 20:24 +0200, Eduard Zingerman wrote: > [...] > > > > + case STACK_MISC: > > > + case STACK_INVALID: > > > + /* Create an invalid entry for MISC and INVALID */ > > > + ret = gen_exception_frame_desc_reg_entry(env, ¬_init_reg, off, frame->frameno); > > > + if (ret < 0) > > > + return 0; > > > > No tests are failing if I comment out this block. > > Looking at the merge_frame_desc() logic it appears to me that fd > > entries with fd->type == NOT_INIT would only be merged with other > > NOT_INIT entries. What is the point of having such entries at all? > > Oh, I got it, it's a mark that ensures that no merge would happen with > e.g. some resource pointer. Makes sense, sorry for the noise. > > Still, I think that my STACK_ZERO remark was correct. I will add tests to exercise these cases, but the idea for STACK_ZERO was to treat it as if we had a NULL value in that stack slot, thus allowing merging with other resource pointers. Sometimes when NULL initializing something, it can be marked as STACK_ZERO in the verifier state. Therefore, we would prefer to treat it same as the case where a scalar reg known to be zero is spilled to the stack (that is what we do by using a fake struct bpf_reg_state).
