On Mon, Jul 15, 2024 at 10:34 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote: > > On Mon, 2024-07-15 at 18:51 -0700, Alexei Starovoitov wrote: > > On Mon, Jul 15, 2024 at 4:02 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote: > > > > > > @@ -21771,6 +22058,12 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr, __u3 > > > if (ret == 0) > > > ret = check_max_stack_depth(env); > > > > > > + /* might decrease stack depth, keep it before passes that > > > + * allocate additional slots. > > > + */ > > > + if (ret == 0) > > > + ret = remove_nocsr_spills_fills(env); > > > > Probably should be before check_max_stack_depth() above :) > > I thought about it, unfortunately, that would be a half-measure. > There are two places where verifier reports stack depth errors: > - check_stack_access_within_bounds() checks for access outside > [-MAX_BPF_STACK..0) region within one subprogram; > - check_max_stack_depth() checks accumulated stack depth across > subprogram calls. > > It is possible to move remove_nocsr_spills_fills() before > check_max_stack_depth(), but check_stack_access_within_bounds() would > still report errors for nocsr stack slots, because > check_nocsr_stack_contract() and check_stack_access_within_bounds() > are both invoked during main verification pass and contract validation > is not yet finished. Agree that it's a half measure, but it's still better than doing it after check_max_stack_depth(). We can also allow check_stack_access_within_bounds() to go above 512 for nocsr pattern. If spill/fill is later removed then great, if not then it's not a big deal to go slightly above 512 especially considering that private stack is coming in soon.
