On Mon, Nov 4, 2024 at 11:35 AM Yonghong Song <yonghong.song@xxxxxxxxx> wrote:
> @@ -6070,11 +6105,23 @@ static int check_max_stack_depth_subprog(struct bpf_verifier_env *env, int idx,
> depth);
> return -EACCES;
> }
> - depth += round_up_stack_depth(env, subprog[idx].stack_depth);
> + subprog_depth = round_up_stack_depth(env, subprog[idx].stack_depth);
> + depth += subprog_depth;
> if (depth > MAX_BPF_STACK && !*subtree_depth) {
> *subtree_depth = depth;
> *depth_frame = frame + 1;
> }
> + if (priv_stack_supported != NO_PRIV_STACK) {
> + if (!subprog[idx].use_priv_stack) {
> + if (subprog_depth > MAX_BPF_STACK) {
> + verbose(env, "stack size of subprog %d is %d. Too large\n",
> + idx, subprog_depth);
> + return -EACCES;
> + }
> + if (subprog_depth >= BPF_PRIV_STACK_MIN_SIZE)
> + subprog[idx].use_priv_stack = true;
> + }
> + }
Hold on. If I'm reading this correctly this adaptive priv stack
concept will make some subprogs with stack >= 64 to use priv_stack
while other subprogs will still use normal stack?
Same for the main prog. It may or may not use priv stack ?
I guess this is ok-ish, but needs to be clearly explained in comments
and commit log.
My first reaction to such adaptive concept was negative, since
such "random" mix of priv stack in some subprogs makes
the whole thing pretty hard to reason about it,
but I guess it's valid to use normal stack when stack usage
is small. No need to penalize every subprog.
I wonder what others think about it.
Also it would be cleaner to rewrite above as:
if (subprog_depth > MAX_BPF_STACK) {
verbose();
return -EACCESS;
}
if (priv_stack_supported == PRIV_STACK_ADAPTIVE &&
subprog_depth >= BPF_PRIV_STACK_MIN_SIZE)
subprog[idx].use_priv_stack = true;
less indent and easier to read.
