On Tue, Dec 5, 2023 at 3:21 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote: > > On Mon, 2023-12-04 at 15:39 -0800, Andrii Nakryiko wrote: > [...] > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index 16d5550eda4d..642260d277ce 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -19899,18 +19899,6 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog) > > /* 1st arg to a function */ > > regs[BPF_REG_1].type = PTR_TO_CTX; > > mark_reg_known_zero(env, regs, BPF_REG_1); > > - ret = btf_check_subprog_arg_match(env, subprog, regs); > > Not sure if this is important or not. btf_check_subprog_arg_match() > might have set 'func_info_aux[subprog].unreliable = true'. > bpf_check_attach_target() checks this flag for subprograms that are > being replaced, and seem to be ok accepting 'subprog == 0'. > This change makes it so .unreliable is never set for 'subprog == 0'. > True, I missed this corner case. But I'm now wondering if it is actually better to have a dedicated check just for entry program which explicitly checks that we have one argument and it's a PTR_TO_CTX (taking into account tags as well). btf_check_subprog_arg_match() business is way too indirect and subtle, IMO. I'll think a bit more and see what's the best way forward, thanks for spotting this! > [...] > >
