linux-kernel - Re: [PATCH] uio: disable lazy irq disable to avoid double fire

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: <6d5b0844-dcaf-902e-466a-1af5ca340b1e@gmail.com>
Date:   Sat, 23 May 2020 11:26:20 +0200
From:   Thommy Jakobsson <thommyj@...il.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     linux-kernel@...r.kernel.org
Subject: Re: [PATCH] uio: disable lazy irq disable to avoid double fire

On 2020-05-22 11:14, Greg KH wrote:
> On Thu, May 21, 2020 at 04:42:09PM +0200, Thommy Jakobsson wrote:
>> +	if (uioinfo->irq) {
> 
> How is this not true at this point in time based on the code above this?
> ->irq should always be set here, right?
It seems to me like there is a path to continue without an IRQ in the
section above, "uioinfo->irq = UIO_IRQ_NONE;", where UIO_IRQ_NONE is
0. Do you agree?

> 
>> +		struct irq_data *irq_data = irq_get_irq_data(uioinfo->irq);
>> +
>> +		/*
>> +		 * If a level interrupt, dont do lazy disable. Otherwise the
>> +		 * irq will fire again since clearing of the actual cause, on
>> +		 * device level, is done in userspace
>> +		 */
>> +		if (!irq_data) {
>> +			dev_err(&pdev->dev, "unable to get irq data\n");
>> +			ret = -ENXIO;
>> +			goto bad1;
> 
> Why is not having this information all of a sudden an error for this
> code?  What could cause that info to not be there?  Existing systems
> without this set would work just fine before this change, and I think
> this breaks them, right?
irq_data should always exists as long as there is an irq descriptor and
I assumed that the descriptors "should" exists at the point when this
code run. So a NULL would either mean something serious and would be
more of a sanity check than anything else, or (more likely) it is the
wrong irq configured.

The "should" comes from that this code path later registers the irq
(devm_uio_register_device->... ->__uio_register_device->...
->request_irq->... ->request_threaded_irq), and when this happen its an
-EINVAL back if there isn't any descriptor from irq_to_desc (which is
the same function as irq_get_data internally uses). So I don't see any
new breaks from this, but I could be wrong so please correct me in that
case. At least it was my intention to not break anything currently
working. Maybe it is better to just do a dev_dbg and let
request_threaded_irq handle the wrong irq case?

> 
>> +		}
>> +		/*
>> +		 * irqd_is_level_type() isn't used since isn't valid unitil
>> +		 * irq is configured.
>> +		 */
>> +		if (irqd_get_trigger_type(irq_data) & IRQ_TYPE_LEVEL_MASK) {
>> +			dev_info(&pdev->dev, "disable lazy unmask\n");
> 
> Why dev_info?  If drivers are working properly, they should be quiet.
> dev_dbg() is fine to use here if you want to debug things to see what is
> happening.
Agreed

BR,
Thommy Jakobsson