linux-kernel - Re: [PATCH] iommu/arm-smmu-v3: Implement shutdown method

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 for Android: free password hash cracker in your pocket

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

Date:   Fri, 30 Jun 2017 18:11:03 +0100
From:   Will Deacon <will.deacon@....com>
To:     Nate Watterson <nwatters@...eaurora.org>
Cc:     Robin Murphy <robin.murphy@....com>,
        Joerg Roedel <joro@...tes.org>,
        linux-arm-kernel@...ts.infradead.org,
        iommu@...ts.linux-foundation.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iommu/arm-smmu-v3: Implement shutdown method

On Thu, Jun 29, 2017 at 06:20:00PM -0400, Nate Watterson wrote:
> On 6/29/2017 2:34 PM, Will Deacon wrote:
> >On Thu, Jun 29, 2017 at 01:40:15PM -0400, Nate Watterson wrote:
> >>The shutdown method disables the SMMU and its interrupts to avoid
> >>potentially corrupting a new kernel started with kexec.
> >>
> >>Signed-off-by: Nate Watterson <nwatters@...eaurora.org>
> >>---
> >>  drivers/iommu/arm-smmu-v3.c | 11 +++++++++++
> >>  1 file changed, 11 insertions(+)
> >
> >We should update arm-smmu.c as well.
> >
> >>diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> >>index 380969a..907d576 100644
> >>--- a/drivers/iommu/arm-smmu-v3.c
> >>+++ b/drivers/iommu/arm-smmu-v3.c
> >>@@ -2765,9 +2765,19 @@ static int arm_smmu_device_remove(struct platform_device *pdev)
> >>  	struct arm_smmu_device *smmu = platform_get_drvdata(pdev);
> >>  	arm_smmu_device_disable(smmu);
> >>+
> >>+	/* Disable IRQs */
> >>+	arm_smmu_write_reg_sync(smmu, 0, ARM_SMMU_IRQ_CTRL,
> >>+				ARM_SMMU_IRQ_CTRLACK);
> >>+
> >
> >Can you justify the need for this? If we actually need to disable
> >interrupts, then I'd like to understand why so that we can make sure we
> >get the ordering right with respect to disabling the device. Also, do we
> >need to clear the MSI registers too?
> 
> I have no justification. Based on the number of drivers that take care
> to prevent their HW from generating an interrupt, I thought it would be
> required, but I can't find any such requirement explicitly laid out in
> the documentation.
> 
> When you mention the MSI registers do you mean, for instance,
> SMMU_GERROR_IRQ_CFG0? It looks like those are always cleared while
> initializing the SMMU so the case where an SMMU transitions from using
> MSIs to using wired interrupts between kernels will be handled properly.

Sure, but if it's only the re-init path you're concerned about, then I
don't think we have a problem for wired interrupts either. They're masked
at the GIC until we do request_irq, and our handler can tolerate a spurious
IRQ anyway.

I assumed the race you were trying to close was an IRQ firing after we've
disabled the device, but actually, I think the GIC is the right place to
handle that too because propagation delay can cause late IRQs anyway.

So unless there's a case I'm missing, let's not confuse the code by
disabling IRQs for the sake of it.

Will