linux-kernel - Re: [PATCH v4 1/2] PCI: Add tango MSI controller support

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]

Message-ID: <aec89b9a-8377-ff39-547c-9e6eaf254fab@free.fr>
Date:   Tue, 23 May 2017 21:15:16 +0200
From:   Mason <slash.tmp@...e.fr>
To:     Robin Murphy <robin.murphy@....com>,
        Bjorn Helgaas <helgaas@...nel.org>
Cc:     Marc Gonzalez <marc_gonzalez@...madesigns.com>,
        Marc Zyngier <marc.zyngier@....com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Jiang Liu <jiang.liu@...ux.intel.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Liviu Dudau <liviu.dudau@....com>,
        David Laight <david.laight@...lab.com>,
        linux-pci <linux-pci@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        Thibaud Cornic <thibaud_cornic@...madesigns.com>,
        Phuong Nguyen <phuong_nguyen@...madesigns.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 1/2] PCI: Add tango MSI controller support

On 23/05/2017 20:03, Robin Murphy wrote:
> On 23/05/17 18:54, Mason wrote:
>> On 23/05/2017 19:03, Bjorn Helgaas wrote:
>>> On Wed, May 17, 2017 at 04:56:08PM +0200, Marc Gonzalez wrote:
>>>> On 20/04/2017 16:28, Marc Gonzalez wrote:
>>>>
>>>>> +static int tango_set_affinity(struct irq_data *data,
>>>>> +		const struct cpumask *mask, bool force)
>>>>> +{
>>>>> +	return -EINVAL;
>>>>> +}
>>>>> +
>>>>> +static struct irq_chip tango_chip = {
>>>>> +	.irq_ack		= tango_ack,
>>>>> +	.irq_mask		= tango_mask,
>>>>> +	.irq_unmask		= tango_unmask,
>>>>> +	.irq_set_affinity	= tango_set_affinity,
>>>>> +	.irq_compose_msi_msg	= tango_compose_msi_msg,
>>>>> +};
>>>>
>>>> Hmmm... I'm wondering why .irq_set_affinity is required.
>>>>
>>>> static int setup_affinity(struct irq_desc *desc, struct cpumask *mask)
>>>> first calls __irq_can_set_affinity() to check whether
>>>> desc->irq_data.chip->irq_set_affinity) exists.
>>>>
>>>> then calls irq_do_set_affinity(&desc->irq_data, mask, false);
>>>> which calls chip->irq_set_affinity(data, mask, force);
>>>> = msi_domain_set_affinity()
>>>> which calls parent->chip->irq_set_affinity() unconditionally.
>>>>
>>>> Would it make sense to test that the callback is implemented
>>>> before calling it?
>>>>
>>>>
>>>> [    0.723895] Unable to handle kernel NULL pointer dereference at virtual address 00000000
>>>
>>> I'm not sure what you're asking.
>>>
>>> Is this a bug report for the v4 tango driver?
>>
>> No.
>>
>>> Or are you asking whether msi_domain_set_affinity() should be changed
>>> to check whether parent->chip->irq_set_affinity is implemented?
>>
>> Yes. The way things are implemented now, drivers are forced
>> to define an irq_set_affinity callback, even if it just returns
>> an error, otherwise, the kernel crashes, because of the
>> unconditional function pointer deref. 
>>
>>> msi_domain_set_affinity() has called parent->chip->irq_set_affinity()
>>> without checking since it was added in 2014 by f3cf8bb0d6c3 ("genirq: Add
>>> generic msi irq domain support"), so if there's a problem here, it's most
>>> likely in the tango code.
>>
>> The issue is having to define an "empty" function.
>> (Unnecessary code bloat and maintenance.)
> 
> AFAICS, only one driver (other than this one) implements a "do nothing"
> set_affinity callback - everyone else who doesn't do anything hardware
> specific just passes it along via irq_chip_set_affinity_parent().

I counted 4. Where did I mess up?

advk_msi_set_affinity
altera_msi_set_affinity
nwl_msi_set_affinity
vmd_irq_set_affinity
tango_set_affinity

Regards.