linux-kernel - Re: [PATCH v2 1/3] genirq/affinity: Add irq_update_affinity

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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 23 Nov 2020 12:54:45 +0000
From:   John Garry <john.garry@...wei.com>
To:     Marc Zyngier <maz@...nel.org>
CC:     Thomas Gleixner <tglx@...utronix.de>, <gregkh@...uxfoundation.org>,
        <rafael@...nel.org>, <martin.petersen@...cle.com>,
        <jejb@...ux.ibm.com>, <linuxarm@...wei.com>,
        <linux-scsi@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/3] genirq/affinity: Add irq_update_affinity_desc()

Hi Marc,

>>> Right, but if the driver is removed then the interrupts should be
>>> deallocated, right?
>>>
>>
>> When removing the driver we just call free_irq(), which removes the
>> handler and disables the interrupt.
>>
>> But about the irq_desc, this is created when the mapping is created in
>> irq_create_fwspec_mapping(), and I don't see this being torn down in
>> the driver removal, so persistent in that regard.
> 
> If the irq_descs are created via the platform_get_irq() calls in
> platform_get_irqs_affinity(), I'd expect some equivalent helper to
> tear things down as a result, calling irq_dispose_mapping() behind the
> scenes.
> 

So this looks lacking in the kernel AFAICS...

So is there a reason for which irq dispose mapping is not a requirement 
for drivers when finished with the irq? because of shared interrupts?

>> So for pci msi I can see that we free the irq_desc in
>> pci_disable_msi() -> free_msi_irqs() -> msi_domain_free_irqs() ...
>>
>> So what I am missing here?
> 
> I'm not sure the paths are strictly equivalent. On the PCI side, we
> can have something that completely driver agnostic, as it is all
> architectural. In your case, only the endpoint driver knows about what
> happens, and needs to free things accordingly.
> 
> Finally, there is the issue in your driver that everything is
> requested using devm_request_irq, which cannot play nicely with an
> explicit irq_desc teardown. You'll probably need to provide the
> equivalent devm helpers for your driver to safely be taken down.
> 

Yeah, so since we use the devm irq request method, we also need a devm 
dispose release method as we can't dispose the irq mapping in the 
remove() method, prior to the irq_free() in the later devm release method.

But it looks like there is more to it than that, which I'm worried is 
far from non-trivial. For example, just calling irq_dispose_mapping() 
for removal and then plaform_get_irq()->acpi_get_irq() second time fails 
as it looks like more tidy-up is needed for removal...

Cheers,
John