Re: [PATCH RFC] arm64: entry: PSTATE_I_SET is leaking on pseudo NMI mode

[Vladimir Murzin <vladimir.murzin@arm.com>]

On 8/10/26 16:05, Will Deacon wrote:
> On Mon, Aug 10, 2026 at 01:42:15PM +0100, Vladimir Murzin wrote:
>> On 8/10/26 12:43, Will Deacon wrote:
>>> On Fri, Aug 07, 2026 at 09:29:12AM -0700, Breno Leitao wrote:
>>>> On Fri, Aug 07, 2026 at 07:58:21AM -0700, Breno Leitao wrote:
>>>>> Meanwhile, I will try to ftrace the writes to PMR and regs->pmr to get
>>>>> a better grasp of the states machine we are in (probably on Monday).
>>>> It seems LLM found a very easy to reproduce this:
>>>>
>>>> 	bash-5.1# dmesg
>>>>
>>>> 	bash-5.1#  cd /sys/kernel/tracing
>>>> 	echo 'r:pmr vfs_read bad=+0($retval):u64' >> kprobe_events
>>>> 	echo 1 > events/kprobes/pmr/enable
>>> Nice, that triggers straightforwardly in QEMU for me. The diff below
>>> (which implements my suggestion from [1]) seems to fix the issue, but
>>> it would be good to hear feedback from one of the Arm folks.
>>>
>>> [1] https://lore.kernel.org/all/anXgWRmcjwPKG7N5@willie-the-truck/
>>>
>>> --->8
>>>
>>> diff --git a/arch/arm64/include/asm/daifflags.h b/arch/arm64/include/asm/daifflags.h
>>> index 795b35128467..691ee5f86dbe 100644
>>> --- a/arch/arm64/include/asm/daifflags.h
>>> +++ b/arch/arm64/include/asm/daifflags.h
>>> @@ -132,7 +132,7 @@ static __always_inline void local_daif_inherit(struct pt_regs *regs)
>>>                 trace_hardirqs_on();
>>>
>>>         if (system_uses_irq_prio_masking())
>>> -               gic_write_pmr(regs->pmr);
>>> +               gic_write_pmr(regs->pmr & ~GIC_PRIO_PSR_I_SET);
>>>
>>>         /*
>>>          * We can't use local_daif_restore(regs->pstate) here as
>>>
>> I have no strong opinion on the change, but I struggle to see how it
>> fits into the big picture.
>>
>> Specifically, I have difficulty explaining this change in isolation.
>> Everywhere else, we try to keep DAIF.I and GIC_PRIO_PSR_I_SET in sync,
>> so it is not clear why we should allow them to go out of sync when
>> inheriting the exception state from the previous context.
> Yeah, I think you're right, and looking at it some more it means we
> end up with the pmr in the IRQON state which will break
> arch_irqs_disabled().
> 

Ahh, nice catch!

>> At the same time, IIUC, the only reason we call local_irq_disable()
>> (which also causes the states to become unsynchronized) in
>> arm64_exit_to_kernel_mode() is for preemption path. So avoiding
>> local_irq_disable() (and preemption) when we interrupted a
>> non-preemptible context seems easier to follow.
> In the past, we only preempted when returning to EL1 off the back of an
> IRQ, but that was changed in ae654112eac0 ("arm64: entry: Use split
> preemption logic") which I think is where this bug was introduced.
>

I believe we started allowing preemption from EL1 exceptions other
than IRQ when we moved to the generic IRQ entry code. It turned out
that, unfortunately, this is not safe in some cases, which is what
Mark's series addressed.

I agree that the change mentioned above introduced an unconditional
local_irq_disable(), and if we want to backport it, it should be
recorded under a Fixes.
 > So we could probably hack something as you suggest, but maybe the best
> option is to take patches 8 and 9 from your FEAT_NMI series? WDYT?
> 

Whatever you prefer, tbh :)

Patch 9 codifies my understanding of how things are expected to
work. That said, I accept that I could be missing something, hence the
RFC tag.

In case you decide to take it, we probably need to:

- extend the commit message to cover Breno's report (right now, the
  commit message makes it look like a refactoring)

- add Fixes tags if we want it to be backported

Alternatively, I can respin the patch separately with the suggestions
above applied - just let me know ;)

Cheers
Vladimir

> Will
>