linux-kernel - Re: [PATCH 15/18] irqchip/apple-aic: Add support for the Apple Interrupt Controller

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <0a1a9d20-ad4e-27e8-0356-567d519653cc@marcan.st>
Date:   Mon, 8 Feb 2021 20:13:22 +0900
From:   Hector Martin 'marcan' <marcan@...can.st>
To:     Arnd Bergmann <arnd@...nel.org>, Marc Zyngier <maz@...nel.org>
Cc:     SoC Team <soc@...nel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        Rob Herring <robh+dt@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        DTML <devicetree@...r.kernel.org>,
        Olof Johansson <olof@...om.net>
Subject: Re: [PATCH 15/18] irqchip/apple-aic: Add support for the Apple
 Interrupt Controller

On 08/02/2021 19.29, Arnd Bergmann wrote:
> On Mon, Feb 8, 2021 at 10:25 AM Marc Zyngier <maz@...nel.org> wrote:
>> On Thu, 04 Feb 2021 20:39:48 +0000, Hector Martin <marcan@...can.st> wrote:
> 
>>> +{
>>> +     return readl(ic->base + reg);
>>
>> Please consider using the _relaxed accessors, as I don't think any of
>> these interacts with memory (apart from IPIs, of course).
> 
> MSI interrupts require serializing with DMA, so at the minimum I think there
> needs to be something that ensures that DMA from device into memory
> has completed before delivering the completion interrupt to a driver. This
> may already be implied when the AIC is entered, but this is hard to know
> without actual hardware specs.
> 

I don't think this can be implied in any case, because if IRQ A fires 
and then the CPU speculates its way through AIC into the IRQ B handler, 
which reads DMA'd memory, then IRQ B fires and it has higher priority 
and *that* is what ends up getting returned from the event register 
first, the execution will commit with an ordering violation.

I'm pretty sure we need *some* level of explicit synchronization between 
reading the event register and actually delivering IRQs downstream. 
Using _relaxed might be okay, but we'd still need something where the 
isb() currently is in aic_handle_irq (though I admit I don't have a 
perfect picture of the memory ordering subtleties involved here yet).

Incidentally, just from the races and problems I've run into with 
trivial tests in m1n1, these CPUs seem to be *very* eager to speculate 
and I suspect they will help uncover race conditions in Linux...

-- 
Hector Martin "marcan" (marcan@...can.st)
Public Key: https://mrcn.st/pub