linux-kernel - Re: [PATCH] irqchip/gic-v3-its: Don't try to move a disabled irq

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

Date:   Thu, 11 Jun 2020 17:44:45 +0000
From:   "Saidi, Ali" <alisaidi@...zon.com>
To:     Thomas Gleixner <tglx@...utronix.de>,
        "Herrenschmidt, Benjamin" <benh@...zon.com>,
        "maz@...nel.org" <maz@...nel.org>
CC:     "jason@...edaemon.net" <jason@...edaemon.net>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "Woodhouse, David" <dwmw@...zon.co.uk>,
        "Zilberman, Zeev" <zeev@...zon.com>,
        "Machulsky, Zorik" <zorik@...zon.com>
Subject: Re: [PATCH] irqchip/gic-v3-its: Don't try to move a disabled irq


On 6/8/20, 8:49 AM, "Thomas Gleixner" <tglx@...utronix.de> wrote:
    
    
    "Herrenschmidt, Benjamin" <benh@...zon.com> writes:
    > On Wed, 2020-06-03 at 16:16 +0100, Marc Zyngier wrote:
    >> > My original patch should certain check activated and not disabled.
    >> > With that do you still have reservations Marc?
    >>
    >> I'd still prefer it if we could do something in core code, rather
    >> than spreading these checks in the individual drivers. If we can't,
    >> fair enough. But it feels like the core set_affinity function could
    >> just do the same thing in a single place (although the started vs
    >> activated is yet another piece of the puzzle I didn't consider,
    >> and the ITS doesn't need the "can_reserve" thing).
    >
    > For the sake of fixing the problem in a timely and backportable way I
    > would suggest first merging the fix, *then* fixing the core core.
    
    The "fix" is just wrong
    
    >       if (cpu != its_dev->event_map.col_map[id]) {
    >               target_col = &its_dev->its->collections[cpu];
    > -             its_send_movi(its_dev, target_col, id);
    > +
    > +             /* If the IRQ is disabled a discard was sent so don't move */
    > +             if (!irqd_irq_disabled(d))
    
    That check needs to be !irqd_is_activated() because enable_irq() does
    not touch anything affinity related.
    
    > +                     its_send_movi(its_dev, target_col, id);
    > +
    >               its_dev->event_map.col_map[id] = cpu;
    >               irq_data_update_effective_affinity(d, cpumask_of(cpu));
    
    And then these associtations are disconnected from reality in any case.
    
    Something like the completely untested patch below should work.

I've been unable to reproduce the problem with your patch on an Arm system.

Thanks,

Ali