linux-kernel - Re: [PATCH v3 7/8] x86/irq: WARN_ONCE() if irq_move

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:   Thu, 20 May 2021 16:00:44 +0200
From:   Thomas Gleixner <tglx@...utronix.de>
To:     "H. Peter Anvin" <hpa@...or.com>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        x86@...nel.org
Subject: Re: [PATCH v3 7/8] x86/irq: WARN_ONCE() if irq_move_cleanup is called on a pending interrupt

On Wed, May 19 2021 at 14:21, H. Peter Anvin wrote:
> The current IRQ vector allocation code should be "clean" and never
> issue a IRQ_MOVE_CLEANUP_VECTOR IPI for an interrupt that could still
> be pending. This should make it possible to move it to the "normal"
> system IRQ vector range. This should probably be a three-step process:
>
> 1. Introduce this WARN_ONCE() on this event ever occurring.
> 2. Remove the self-IPI hack.
> 3. Move the IRQ_MOVE_CLEANUP_VECTOR to the sysvec range.

Second thoughts on this.

Despite having a halfways sane mechanism now, that warning still can be
triggered for remapped interrupts which can be moved from task context.

Interrupt is bound to CPU1 and moved to CPU2.

CPU0               CPU1                 CPU2            Device

set_affinity()     local_irq_disable()
                                                        Raise interrupt
                      -> pending in IRR                 on CPU1
  remap to CPU2
  (after this point all interrupts will go to CPU2)

                                                        Raise interrupt
                                                        on CPU2
                                        handle_irq()
                                        send_cleanup_ipi()

                   local_irq_enable()
                   handle_irq()
                   handle_cleanup_ipi()

Now if we move the cleanup vector up (higher priority) then CPU1 will
have:

                   local_irq_enable()
                   handle_cleanup_ipi()
                      WARN(irq set in IRR)

Unlikely but bound to happen.

Adding the WARN_ON() to the current code is harmless because it can't
trigger. Let me think some more about that.

Thanks,

        tglx