Re: [PATCH] arm64: archrandom: avoid trapping ID register read in __cpu_has_rng()

> On 4 Aug 2026, at 15:56, Will Deacon <[email protected]> wrote:
> 
> On Fri, Jul 31, 2026 at 06:26:44PM +0100, Aman Priyadarshi wrote:
>>> On 31 Jul 2026, at 15:43, Will Deacon <[email protected]> wrote:
>>> On Mon, Jul 20, 2026 at 03:06:15PM +0100, Aman Priyadarshi wrote:
>>>> diff --git a/arch/arm64/include/asm/archrandom.h b/arch/arm64/include/asm/archrandom.h
>>>> index 8babfbe31f95..8067e9a35641 100644
>>>> --- a/arch/arm64/include/asm/archrandom.h
>>>> +++ b/arch/arm64/include/asm/archrandom.h
>>>> @@ -61,8 +61,22 @@ static inline bool __arm64_rndrrs(unsigned long *v)
>>>> 
>>>> static __always_inline bool __cpu_has_rng(void)
>>>> {
>>>> - if (unlikely(!system_capabilities_finalized() && !preemptible()))
>>>> - return this_cpu_has_cap(ARM64_HAS_RNG);
>>>> + if (unlikely(!system_capabilities_finalized() && !preemptible())) {
>>>> + /*
>>>> + * Until the ARM64_HAS_RNG alternative is patched we can't use
>>>> + * the static-branch form, so consult the feature register
>>>> + * directly. Don't use this_cpu_has_cap() here: it reads
>>>> + * ID_AA64ISAR0_EL1 from hardware on every call, under
>>>> + * virtualization each ID register read traps to the hypervisor
>>>> + * (HCR_EL2.TID3) -- producing a storm of vmexits during boot.
>>>> + * The sanitised value is cached in memory.
>>>> + */
>>>> + u64 isar0 = read_sanitised_ftr_reg(SYS_ID_AA64ISAR0_EL1);
>>>> +
>>>> + return cpuid_feature_extract_unsigned_field(isar0,
>>>> + ID_AA64ISAR0_EL1_RNDR_SHIFT) >=
>>>> + ID_AA64ISAR0_EL1_RNDR_IMP;
>>>> + }
>>> 
>>> You can probably rewrite this a little more cleanly along the lines of
>>> the (not even compile-tested) diff below. I was about to do that, but
>>> then I got a bit confused by the whole thing. The preemptible() check is
>>> presumably not needed if we're accessing the in-memory feature registers
>>> rather than the per-CPU id registers, but then how do you handle races
>>> with concurrent updates to the "safe value" made by CPUs concurrently
>>> coming online?
>> 
>> I kept preemptible() check for this exact reason: the updates made by CPUs
>> concurrently coming online will always take the downgrade path (a secondary
>> CPU can clear RNDR, never set it), and therefore by taking the non-preemptible
>> branch I can guarantee that the pinned CPU supports the said feature.
>> I agree, this assumes that a secondary CPU folds its own ID registers into sys_val
>> before it can ever be a randomness consumer, but looking at the code that seems
>> the case, please feel free to correct me.
>> Besides, in my opinion, it's hard to argue correctness of this code without
>> preemptible() check.
> 
> My point is that this change introduces a data race on 'reg->sys_val' for
> the ID_AA64ISAR0_EL1 entry in the arm64_ftr_regs array.
> 
> Will

Agreed, you're right. My reasoning was that existing read_sanitised_ftr_reg() callers
already race with sys_val updates during hotplug CPU bringup, so this wasn't a
new problem. But I agree it's not much of a defence.
It looks like an easy fix, though: mark the reader and the writer. What do you think
of the patch below? I'm happy to post it as a separate patch ahead of this fix once
you confirm it works for you.

- Aman Priyadarshi

--->8

diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 9a22df0c5120..e1c10a23da3c 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -1235,18 +1235,20 @@ void __init init_cpu_features(struct cpuinfo_arm64 *info)
 static void update_cpu_ftr_reg(struct arm64_ftr_reg *reg, u64 new)
 {
        const struct arm64_ftr_bits *ftrp;
+       u64 sys_val = reg->sys_val;
 
        for (ftrp = reg->ftr_bits; ftrp->width; ftrp++) {
-               s64 ftr_cur = arm64_ftr_value(ftrp, reg->sys_val);
+               s64 ftr_cur = arm64_ftr_value(ftrp, sys_val);
                s64 ftr_new = arm64_ftr_value(ftrp, new);
 
                if (ftr_cur == ftr_new)
                        continue;
                /* Find a safe value */
                ftr_new = arm64_ftr_safe_value(ftrp, ftr_new, ftr_cur);
-               reg->sys_val = arm64_ftr_set_value(ftrp, reg->sys_val, ftr_new);
+               sys_val = arm64_ftr_set_value(ftrp, sys_val, ftr_new);
        }
 
+       WRITE_ONCE(reg->sys_val, sys_val);
 }
 
 static int check_update_ftr_reg(u32 sys_id, int cpu, u64 val, u64 boot)
@@ -1526,7 +1528,8 @@ u64 read_sanitised_ftr_reg(u32 id)
 
        if (!regp)
                return 0;
-       return regp->sys_val;
+
+       return READ_ONCE(regp->sys_val);
 }
 EXPORT_SYMBOL_GPL(read_sanitised_ftr_reg);