linux-kernel - Re: [PATCH] irq: fix the interrupt trigger type override issue

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: <CAJNi4rPm2N=_H7LjT3X3_b2-UVZP+EEOw-ztwBwWBs=vMtfYQQ@mail.gmail.com>
Date: Mon, 2 Sep 2024 16:42:28 +0800
From: richard clark <richard.xnu.clark@...il.com>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, 
	torvalds@...ux-foundation.org
Subject: Re: [PATCH] irq: fix the interrupt trigger type override issue

Hi Thomas,

On Mon, Sep 2, 2024 at 3:34 PM Thomas Gleixner <tglx@...utronix.de> wrote:
>
> Richard!
>
> On Mon, Sep 02 2024 at 11:17, Richard Clark wrote:
>
> The subsystem prefix is 'irqdomain' not 'irq'
>
Right, will correct it.
>
> # git log --online $FILE
>
> gives you a decent hint.
>
> > In current implementation, the trigger type in 'flags' when calling request_irq
> > will override the type value get from the firmware(dt/acpi node) if they are
> > not consistent, and the overrided trigger type value will be retained by irq_data,
> > consequently the type value get from the firmware will not match the retained one
> > next time in case the virq is available.
> >
> > Thus below error message will be observed by the __2nd__ 'insmod' within the
> > 'insmod - rmmod - insmod' operation sequence for the same device driver kernel
> > module, in which request_irq(..., IRQ_TYPE_LEVEL_HIGH, ...) is used:
> >
> >       irq: type mismatch, failed to map hwirq-182 for interrupt-controller!
>
> How so?
>
> 1) insmod()
>      irq_create_fwspec_mapping(fwspec)
>        irq_domain_translate(fwspec, ... &type); <- Sets type to the FW value
>
>        virq = irq_find_mapping(domain, hwirq);
>        if (virq) {
>          // Path not taken
>        }
>
>        // Map interrupt
>        ...
>
>        irqd_set_trigger_type(..., type);
>
> 2) rmmod()
>      tears down mapping
>
This just tears down the action allocated and installed by
request_irq(...), but does not teardown the irq's node inserted in the
revmap_tree.
>
> 3) insmod()
>
>       Should be exactly the same as #1 because the previous mapping was
>       torn down by rmmod()
>
Not the same exactly, the {irq, irq_data} will still be in the
revmap_tree, so it will enter another path in this case:
        virq = irq_find_mapping(domain, hwirq);
        if (virq) {
               // will be in this path for the next 'insmod' since the
{virq, irq_data} not be teardowned by free_irq(...) called by rmmod
       }

>
> Even if the first mapping is not torn down by rmmod(), which is a bug in
> itself, then the type is exactly the same as the firmware describes it, no?
>
> So how exactly does that happen what you describe?
>
The logic is if the trigger type specified by request_irq(...) is not
consistent with the firmware one, the request_irq will override the
FW. We need to keep this logic the same as when we insmod the same
kmod next time -- override the FW's too instead of returning a
mismatch type error.
>
> > The corresponding 'interrupts' property of that device node is:
> >       interrupts = <0 150 1>;
> >
> > This commit fixes the above issue by adding a new checker -
> > irqd_trigger_type_was_set:
>
> This commit is equaly redundant as 'This patch'
>
> # git grep 'This patch' Documentation/process/
>
> Also 'new checker' is not really a technical term.
>
Ah, will correct it. Thanks!
>
> Thanks,
>
>         tglx