On Tue, Jul 2, 2024 at 1:38 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> On Mon, 2024-07-01 at 17:41 -0700, Andrii Nakryiko wrote:
>
> [...]
>
[...]
> > > +static bool verifier_inlines_helper_call(struct bpf_verifier_env *env, s32 imm)
> > > +{
> >
> > note that there is now also bpf_jit_inlines_helper_call()
>
> There is.
> I'd like authors of jit inline patches to explicitly opt-in
> for nocsr, vouching that inline patch follows nocsr contract.
> The idea is that people would extend call_csr_mask() as necessary.
>
you are defining a general framework with these changes, though, so
let's introduce a standard and simple way to do this. Say, in addition
to having arch-specific bpf_jit_inlines_helper_call() we can have
bpf_jit_supports_helper_nocsr() or something. And they should be
defined next to each other, so whenever one changes it's easier to
remember to change the other one.
I don't think requiring arm64 contributors to change the code of
call_csr_mask() is the right approach.
> >
> >
> > > + return false;
> > > +}
> > > +
> > > +/* If 'insn' is a call that follows no_caller_saved_registers contract
> > > + * and called function is inlined by current jit, return a mask with
> > > + * 1s corresponding to registers that are scratched by this call
> > > + * (depends on return type and number of return parameters).
> > > + * Otherwise return ALL_CALLER_SAVED_REGS mask.
> > > + */
> > > +static u32 call_csr_mask(struct bpf_verifier_env *env, struct bpf_insn *insn)
> > > +{
> > > + const struct bpf_func_proto *fn;
> > > +
> > > + if (bpf_helper_call(insn) &&
> > > + verifier_inlines_helper_call(env, insn->imm) &&
> >
> > strictly speaking, does nocsr have anything to do with inlining,
> > though? E.g., if we know for sure (however, that's a separate issue)
> > that helper implementation doesn't touch extra registers, why do we
> > need inlining to make use of nocsr?
>
> Technically, alternative for nocsr is for C version of the
> helper/kfunc itself has no_caller_saved_registers attribute.
> Grep shows a single function annotated as such in kernel tree:
> stackleak_track_stack().
> Or, maybe, for helpers/kfuncs implemented in assembly.
Yes, I suppose it's too dangerous to rely on the compiler to not use
some extra register. I guess worst case we can "inline" helper by
keeping call to it intact :)
> Whenever such helpers/kfuncs are added, this function could be extended.
>
> >
> > > + get_helper_proto(env, insn->imm, &fn) == 0 &&
> > > + fn->nocsr)
> > > + return ~get_helper_reg_mask(fn);
> > > +
> > > + return ALL_CALLER_SAVED_REGS;
> > > +}
> > > +
>
> [...]
>
[...]
> > > + if (i == 1)
> > > + return 0;
> > > + if (mark) {
> > > + s = find_containing_subprog(env, t);
> > > + /* can't happen */
> > > + if (WARN_ON_ONCE(s < 0))
> > > + return 0;
> > > + subprog = &env->subprog_info[s];
> > > + subprog->nocsr_stack_off = min(subprog->nocsr_stack_off, off);
> > > + }
> >
> > why not split pattern detection and all this other marking logic? You
> > can return "the size of csr pattern", meaning how many spills/fills
> > are there surrounding the call, no? Then all the marking can be done
> > (if necessary) by the caller.
>
> I'll try recording pattern size for function call,
> so no split would be necessary.
>
ack
> > The question is what to do with zero patter (no spills/fills for nocsr
> > call, is that valid case?)
>
> Zero pattern -- no spills/fills to remove, so nothing to do.
> I will add a test case, but it should be handled by current implementation fine.
yep, thanks
>
> [...]
>
> > > +/* Remove unnecessary spill/fill pairs, members of nocsr pattern.
> > > + * Do this as a separate pass to avoid interfering with helper/kfunc
> > > + * inlining logic in do_misc_fixups().
> > > + * See comment for match_and_mark_nocsr_pattern().
> > > + */
> > > +static int remove_nocsr_spills_fills(struct bpf_verifier_env *env)
> > > +{
> > > + struct bpf_subprog_info *subprogs = env->subprog_info;
> > > + int i, j, spills_num, cur_subprog = 0;
> > > + struct bpf_insn *insn = env->prog->insnsi;
> > > + int insn_cnt = env->prog->len;
> > > +
> > > + for (i = 0; i < insn_cnt; i++, insn++) {
> > > + spills_num = match_nocsr_pattern(env, i);
> >
> > we can probably afford a single-byte field somewhere in
> > bpf_insn_aux_data to remember "csr pattern size" instead of just a
> > true/false fact that it is nocsr call. And so we wouldn't need to do
> > pattern matching again here, we'll just have all the data.
>
> Makes sense.
>
> [...]
