Re: [PATCH 02/17] arm64: Rework exception masking into abstract logical mask

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] The `pmr` and `allint` fields of `struct exception_mask` are left uninitialized when hardware features are disabled, causing Undefined Behavior and KMSAN panics.
- [High] Missing `__always_inline` on exception mask helpers leads to noinstr violations.
- [High] `arm64_make_errctx_mask` incorrectly masks pseudo-NMIs during SError handling.
- [High] Unnecessary `pmr_sync()` (DSB barrier) executes unconditionally on masked restore paths.

Pre-existing issues:
- [High] Missing `isb()` after writing to the control-plane system register `mdscr_el1`.
--

commit 464598f1f40c66ead1ff67f84cc61270622c7aa4
Author: Jinjie Ruan <ruanjinjie@huawei.com>

arm64: Rework exception masking into abstract logical mask

The current pseudo-NMI implementation in arm64 bodges the GICv3 PMR state
into the DAIF management code. Introduce a new abstract exception_mask
that explicitly treats DAIF, PMR, and ALLINT as separate elements to cleanly
track the exact hardware masking state.

Link: https://lore.kernel.org/linux-arm-kernel/Y5c9SLeJacLYHmP7@FVFF77S0Q05N/

> diff --git a/arch/arm64/include/asm/exception_masks.h b/arch/arm64/include/asm/exception_masks.h
> new file mode 100644
> index 0000000000000..0a21fb97c3ca8
> --- /dev/null
> +++ b/arch/arm64/include/asm/exception_masks.h
> @@ -0,0 +1,141 @@
[ ... ]
> +static inline struct exception_mask arm64_make_procctx_mask(void)
> +{
> +	struct exception_mask mask;
> +
> +	mask.daif = DAIF_PROCCTX;
> +	if (system_uses_irq_prio_masking())
> +		mask.pmr = GIC_PRIO_IRQON;

[Severity: High]
Does this leave the pmr field in the returned mask uninitialized when
system_uses_irq_prio_masking() is false? Returning this struct by value
might propagate garbage data and trigger use-of-uninitialized-value KMSAN
panics during exception entry paths.

> +
> +	mask.allint = 0;
> +
> +	return mask;
> +}
> +
> +static inline struct exception_mask arm64_make_errctx_mask(void)
> +{
> +	struct exception_mask mask;
> +
> +	mask.daif = DAIF_ERRCTX;
> +	if (system_uses_irq_prio_masking())
> +		mask.pmr = GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET;

[Severity: High]
Could this construction inadvertently mask pseudo-NMIs during system error
handling? By statically setting mask.daif = DAIF_ERRCTX, the I-bit is left
set, which masks NMIs at the CPU PSTATE level. The legacy code cleared the
I-bit and set PMR to GIC_PRIO_IRQOFF to allow NMIs while blocking normal
IRQs.

> +
> +	mask.allint = 0;
> +
> +	return mask;
> +}
[ ... ]
> +/* Mask all exceptions immediately */
> +static inline void local_exception_mask(void)
> +{

[Severity: High]
Should local_exception_mask() be marked __always_inline? If the compiler
decides not to inline it, calling it from noinstr exception entry and exit
paths (like arm64_exit_to_kernel_mode) violates noinstr constraints and
can lead to recursive panics when tracing is enabled.

> +	WARN_ON(system_has_prio_mask_debugging() &&
> +		(read_sysreg_s(SYS_ICC_PMR_EL1) == (GIC_PRIO_IRQOFF |
> +						    GIC_PRIO_PSR_I_SET)));
[ ... ]
> +static inline void local_exception_save_mask(struct exception_mask *mask)
> +{
> +	mask->daif = read_sysreg(daif);
> +	if (system_uses_irq_prio_masking())
> +		mask->pmr = gic_read_pmr();

[Severity: High]
Is it possible this conditionally populates the mask and leaves the pmr field
uninitialized when hardware priority masking is disabled? Since the caller
might return this struct by value, it could propagate uninitialized memory.

> +
> +	mask->allint = 0;
> +}
[ ... ]
> +static inline void local_exception_restore(const struct exception_mask mask)
> +{

[Severity: High]
Could the lack of __always_inline on local_exception_restore() also cause
noinstr violations? Similar to local_exception_mask(), compiler heuristics
deciding to out-of-line this function would break the noinstr invariant
during exception handling.

> +	bool irq_disabled = mask.daif & PSR_I_BIT;
[ ... ]
> +	if (!irq_disabled)
> +		trace_hardirqs_on();
> +
> +	if (system_uses_irq_prio_masking()) {
> +		gic_write_pmr(mask.pmr);
> +		pmr_sync();
> +	}

[Severity: High]
Is this pmr_sync() execution necessary when interrupts remain disabled?
The legacy code placed the synchronization barrier inside the !irq_disabled
branch, as writing to the pmr register to mask interrupts is
architecturally self-synchronizing. Executing a pipeline-draining dsb
instruction unconditionally adds overhead on masked restore paths like
kernel exception exits.

> +
> +	write_sysreg(mask.daif, daif);
[ ... ]
> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
> index 29307642f4c90..5b53034428e46 100644
> --- a/arch/arm64/kernel/debug-monitors.c
> +++ b/arch/arm64/kernel/debug-monitors.c
[ ... ]
> @@ -40,10 +40,11 @@ u8 debug_monitors_arch(void)
>   */
>  static void mdscr_write(u64 mdscr)
>  {
> -	unsigned long flags;
> -	flags = local_daif_save();
> +	struct exception_mask mask;
> +
> +	mask = local_exception_save_and_mask();
>  	write_sysreg(mdscr, mdscr_el1);

[Severity: High]
This is a pre-existing issue, but does mdscr_write() require a subsequent
isb() before returning after writing to the control-plane system register
mdscr_el1? Without synchronization, the cpu pipeline might operate under an
undefined debug configuration for several cycles when reached via debug
mechanisms like ptrace.

> -	local_daif_restore(flags);
> +	local_exception_restore(mask);
>  }
>  NOKPROBE_SYMBOL(mdscr_write);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703100135.2512312-1-ruanjinjie@huawei.com?part=2