Re: [PATCH bpf-next v9 2/2] arm64/cfi,bpf: Support kCFI + BPF on arm64

[Will Deacon <will@kernel.org>]

Hey Sami,

On Fri, Jul 11, 2025 at 11:49:29AM -0700, Sami Tolvanen wrote:
> > > +#define cfi_get_offset cfi_get_offset
> > > +extern u32 cfi_bpf_hash;
> > > +extern u32 cfi_bpf_subprog_hash;
> > > +extern u32 cfi_get_func_hash(void *func);
> > > +#else
> > > +#define cfi_bpf_hash 0U
> > > +#define cfi_bpf_subprog_hash 0U
> > > +static inline u32 cfi_get_func_hash(void *func)
> > > +{
> > > +     return 0;
> > > +}
> > > +#endif /* CONFIG_CFI_CLANG */
> > > +#endif /* _ASM_ARM64_CFI_H */
> >
> > This looks like an awful lot of boiler plate to me. The only thing you
> > seem to need is the CFI offset -- why isn't that just a constant that we
> > can define (or a Kconfig symbol?).
> 
> The cfi_get_offset function was originally added in commit
> 4f9087f16651 ("x86/cfi,bpf: Fix BPF JIT call") because the offset can
> change on x86 depending on which CFI scheme is enabled at runtime.
> Starting with commit 2cd3e3772e41 ("x86/cfi,bpf: Fix bpf_struct_ops
> CFI") the function is also called by the generic BPF code, so we can't
> trivially replace it with a constant. However, since this defaults to
> `4` unless the architecture adds pre-function NOPs, I think we could
> simply move the default implementation to include/linux/cfi.h (and
> also drop the RISC-V version). Come to think of it, we could probably
> move most of this boilerplate to generic code. I'll refactor this and
> send a new version.

Excellent, thanks.

> > > @@ -2009,9 +2018,9 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
> > >               jit_data->ro_header = ro_header;
> > >       }
> > >
> > > -     prog->bpf_func = (void *)ctx.ro_image;
> > > +     prog->bpf_func = (void *)ctx.ro_image + cfi_get_offset();
> > >       prog->jited = 1;
> > > -     prog->jited_len = prog_size;
> > > +     prog->jited_len = prog_size - cfi_get_offset();
> >
> > Why do we add the offset even when CONFIG_CFI_CLANG is not enabled?
> 
> The function returns zero if CFI is not enabled, so I believe it's
> just to avoid extra if (IS_ENABLED(CONFIG_CFI_CLANG)) statements in
> the code. IMO this is cleaner, but I can certainly change this if you
> prefer.

Ah, that caught me out because the !CONFIG_CFI_CLANG stub is in the
core code (and I'm extra susceptible to being caught out on a warm
Friday evening!).

Hopefully if you're able to trim down the boilerplate then this will
become more obvious too.

Cheers,

Will