On Tue, 2024-08-13 at 08:18 -0700, Yonghong Song wrote:
[...]
> > > > @@ -16238,6 +16260,20 @@ static void mark_nocsr_pattern_for_call(struct bpf_verifier_env *env,
> > > > bpf_jit_inlines_helper_call(call->imm));
> > > > }
> > > >
> > > > + if (bpf_pseudo_kfunc_call(call)) {
> > > > + struct bpf_kfunc_call_arg_meta meta;
> > > > + int err;
> > > > +
> > > > + err = fetch_kfunc_meta(env, call, &meta, NULL);
> > > > + if (err < 0)
> > > > + /* error would be reported later */
> > > > + return;
> > > > +
> > > > + clobbered_regs_mask = kfunc_nocsr_clobber_mask(&meta);
> > > > + can_be_inlined = (meta.kfunc_flags & KF_NOCSR) &&
> > > > + verifier_inlines_kfunc_call(&meta);
> > > I think we do not need both meta.kfunc_flags & KF_NOCSR and
> > > verifier_inlines_kfunc_call(&meta). Only one of them is enough
> > > since they test very similar thing. You do need to ensure
> > > kfuncs with KF_NOCSR in special_kfunc_list though.
> > > WDYT?
> > I can remove the flag in favour of verifier_inlines_kfunc_call().
>
> Sounds good to me.
Just one more point. The reason I added the KF_NOCSR was to keep the code
as close to helpers case as possible. For helpers there are two guards:
- verifier_inlines_helper_call() function shared between
mark_nocsr_pattern_for_call() and do_misc_fixups();
- bpf_func_proto->allow_nocsr flag.
The idea is that verifier might inline some functions w/o allowing nocsr.
Hence I decided to use KF_NOCSR in place of bpf_func_proto->allow_nocsr.
On the other hand, verifier_inlines_kfunc_call() is not used by any
other function except mark_nocsr_pattern_for_call() at the moment,
so the KF_NOCSR flag might be redundant indeed.
