On Mon, Jun 13, 2022 at 9:22 AM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote: > > > On Mon, 2022-06-13 at 08:48 -0700, Song Liu wrote: > > > +static int optimize_bpf_loop(struct bpf_verifier_env *env) > > > + new_prog = inline_bpf_loop(env, > > > + i + delta, > > > + -(stack_depth + stack_depth_extra), > > > + inline_state->callback_subprogno, > > > + &cnt); > > > + if (!new_prog) > > > + return -ENOMEM; > > > > We do not fail over for -ENOMEM, which is reasonable. (It is also reasonable if > > we do fail the program with -ENOMEM. However, if we don't fail the program, > > we need to update stack_depth properly before returning, right? > > > > Ouch, you are correct! Sorry, this was really sloppy on my side. The > behavior here should be the same as in `do_misc_fixups` which does > fail in case of -ENOMEM. In order to do the same `optimize_bpf_loop` > should remain as is but the following part of the patch has to be > updated: > > > @@ -15031,6 +15193,9 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr) > > ret = check_max_stack_depth(env); > > > > /* instruction rewrites happen after this point */ > > + if (ret == 0) > > + optimize_bpf_loop(env); > > + > > It should be as follows: > > + if (ret == 0) > + ret = optimize_bpf_loop(env); // added `ret` assignment! > > Not sure if there is a reasonable way to write a test for this case. Some error injection will catch this. But IIUC, we don't have it in the selftests at the moment. > I will add this change and produce the v7 today. Do you see anything > else that should be updated? That's all the issues I can see at the moment. Sorry for not catching this one in earlier versions. Thanks, Song
