On 10/21/24 8:43 PM, Alexei Starovoitov wrote:
On Mon, Oct 21, 2024 at 8:21 PM Yonghong Song <yonghong.song@xxxxxxxxx> wrote:for (int i = 0; i < env->subprog_cnt; i++) { - if (!i || si[i].is_async_cb) { - ret = check_max_stack_depth_subprog(env, i); + check_subprog = !i || (check_priv_stack ? si[i].is_cb : si[i].is_async_cb);why? This looks very suspicious.This is to simplify jit. For example, main_prog <=== main_prog_priv_stack_ptr subprog1 <=== there is a helper which has a callback_fn <=== for example bpf_for_each_map_elem callback_fn subprog2 In callback_fn, we cannot simplify do r9 += stack_size_for_callback_fn since r9 may have been clobbered between subprog1 and callback_fn. That is why currently I allocate private_stack separately for callback_fn. Alternatively we could do callback_fn_priv_stack_ptr = main_prog_priv_stack_ptr + off where off equals to (stack size tree main_prog+subprog1). I can do this approach too with a little more information in prog->aux. WDYT?I see. I think we're overcomplicating the verifier just to be able to do 'r9 += stack' in the subprog. The cases of async vs sync and directly vs kfunc/helper (and soon with inlining of kfuncs) are getting too hard to reason about. I think we need to go back to the earlier approach where every subprog had its own private stack and was setting up r9 = my_priv_stack in the prologue.
Indeed, per private_stack per prog(subprog) will be much simpler.
I suspect it's possible to construct a convoluted subprog that calls itself a limited amount of time and the verifier allows that. I feel it will be easier to detect just that condition in the verifier and fallback to the normal stack.
Yes, I think check_max_stack_depth_subprog() should be able to detect subprog recursion.
