On Tue, Feb 20, 2024 at 11:57:04AM +0000, Marc Zyngier wrote:
> On Tue, 20 Feb 2024 11:20:31 +0000,
> Joey Gouly <joey.gouly@xxxxxxx> wrote:
> >
> > On Mon, Feb 19, 2024 at 09:20:02AM +0000, Marc Zyngier wrote:
> > > The unsuspecting kernel tinkerer can be easily confused into
> > > writing something that looks like this:
> > >
> > > ikey.lo = __vcpu_sys_reg(vcpu, SYS_APIAKEYLO_EL1);
> > >
> > > which seems vaguely sensible, until you realise that the second
> > > parameter is the encoding of a sysreg, and not the index into
> > > the vcpu sysreg file... Debugging what happens in this case is
> >
> > type safety :(
>
> Are you advocating for making everything a struct? Or something else?
No, merely lamenting the situation.
Reviewed-by: Joey Gouly <joey.gouly@xxxxxxx>
>
> >
> > > an interesting exercise in head<->wall interactions.
> > >
> > > As they often say: "Any resemblance to actual persons, living
> > > or dead, or actual events is purely coincidental".
> > >
> > > In order to save people's time, add some compile-time hardening
> > > that will at least weed out the "stupidly out of range" values.
> > > This will *not* catch anything that isn't a compile-time constant.
> > >
> > > Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
> > > ---
> > > arch/arm64/include/asm/kvm_host.h | 9 ++++++++-
> > > 1 file changed, 8 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > > index 181fef12e8e8..a5ec4c7d3966 100644
> > > --- a/arch/arm64/include/asm/kvm_host.h
> > > +++ b/arch/arm64/include/asm/kvm_host.h
> > > @@ -895,7 +895,7 @@ struct kvm_vcpu_arch {
> > > * Don't bother with VNCR-based accesses in the nVHE code, it has no
> > > * business dealing with NV.
> > > */
> > > -static inline u64 *__ctxt_sys_reg(const struct kvm_cpu_context *ctxt, int r)
> > > +static inline u64 *___ctxt_sys_reg(const struct kvm_cpu_context *ctxt, int r)
> >
> > When in doubt, add more underscores!
>
> That's the one true way.
>
> >
> > > {
> > > #if !defined (__KVM_NVHE_HYPERVISOR__)
> > > if (unlikely(cpus_have_final_cap(ARM64_HAS_NESTED_VIRT) &&
> > > @@ -905,6 +905,13 @@ static inline u64 *__ctxt_sys_reg(const struct kvm_cpu_context *ctxt, int r)
> > > return (u64 *)&ctxt->sys_regs[r];
> > > }
> > >
> > > +#define __ctxt_sys_reg(c,r) \
> > > + ({ \
> > > + BUILD_BUG_ON(__builtin_constant_p(r) && \
> > > + (r) >= NR_SYS_REGS); \
> > > + ___ctxt_sys_reg(c, r); \
> > > + })
> >
> > I'm assuming the extra macro layer is to try make __builtin_constant_p() as
> > effective as possible? Otherwise maybe it relies on the compiler inling the
> > ___ctxt_sys_reg() function?
>
> It's not about efficiency. It's about making it *work*. Otherwise,
> lack of inlining will screw you over, and you may not check anything.
Thanks,
Joey
