Re: [RFC bpf-next v1 2/8] bpf: no_caller_saved_registers attribute for helper calls

Andrii Nakryiko
2024-07-02T21:22:49Z

On Tue, Jul 2, 2024 at 2:19 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Tue, 2024-07-02 at 14:09 -0700, Andrii Nakryiko wrote:
>
> [...]
>
> > 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.
>
> I'd change the return value for bpf_jit_inlines_helper_call() to enum,
> to avoid naming functions several times.

Hm... I don't know, feels ugly. nocsr is sort of a dependent but
orthogonal axis, so I'd keep it separate. Imagine we add yet another
property that relies on inlining, but is independent of nocsr. I guess
we can have enum be a bit set, but I'd start simple with two
functions.

>
> [...]
>
> > > > 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 :)
>
> Something like that.
>
> …