On Wed, 2023-12-13 at 10:23 -0800, Andrii Nakryiko wrote:
> On Wed, Dec 13, 2023 at 10:14 AM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
> >
> > On Wed, 2023-12-13 at 10:06 -0800, Andrii Nakryiko wrote:
> > [...]
> >
> > > > > @@ -19944,21 +19945,19 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog)
> > > > > }
> > > > > }
> > > > > } else {
> > > > > + /* if main BPF program has associated BTF info, validate that
> > > > > + * it's matching expected signature, and otherwise mark BTF
> > > > > + * info for main program as unreliable
> > > > > + */
> > > > > + if (env->prog->aux->func_info_aux) {
> > > > > + ret = btf_prepare_func_args(env, 0);
> > > > > + if (ret || sub->arg_cnt != 1 || sub->args[0].arg_type != ARG_PTR_TO_CTX)
> > > > > + env->prog->aux->func_info_aux[0].unreliable = true;
> > > > > + }
> > > >
> > > > Nit: should this return if ret == -EFAULT?
> > > >
> > > >
> > >
> > > no, why? I think the old behavior also didn't fail in this case
> >
> > I think it did, here is an excerpt from the current patch:
>
> Ah, sorry, you meant exit if -EFAULT is returned, not on any error.
> Yes, that was old behavior, but I don't think those conditions can
> ever happen because if func_info_aux is non-null, then we successfully
> passed check_btf_func_early() and check_btf_func() checks, which will
> fail early if those conditions are not satisfied.
>
> So instead of a scary-looking check, I figured it's simpler to just
> mark BTF unreliable and move on with the rest of the logic. The whole
> idea of this check is to do basically optional BTF check, but
> otherwise ignore BTF if it doesn't match our expectation.
Oh, right, all checks are covered indeed.
All good then, sorry for the noise.
