linux-kernel - Re: [PATCHv2 1/5] arm64/entry-common: push the judgement of nmi ahead

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: <20210930133257.GB18258@lakrids.cambridge.arm.com>
Date:   Thu, 30 Sep 2021 14:32:57 +0100
From:   Mark Rutland <mark.rutland@....com>
To:     Pingfan Liu <kernelfans@...il.com>
Cc:     linux-arm-kernel@...ts.infradead.org,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>, Marc Zyngier <maz@...nel.org>,
        Joey Gouly <joey.gouly@....com>,
        Sami Tolvanen <samitolvanen@...gle.com>,
        Julien Thierry <julien.thierry@....com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Yuichi Ito <ito-yuichi@...itsu.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCHv2 1/5] arm64/entry-common: push the judgement of nmi ahead

On Sat, Sep 25, 2021 at 11:39:55PM +0800, Pingfan Liu wrote:
> On Fri, Sep 24, 2021 at 06:53:06PM +0100, Mark Rutland wrote:
> > On Fri, Sep 24, 2021 at 09:28:33PM +0800, Pingfan Liu wrote:
> > > In enter_el1_irq_or_nmi(), it can be the case which NMI interrupts an
> > > irq, which makes the condition !interrupts_enabled(regs) fail to detect
> > > the NMI. This will cause a mistaken account for irq.
> > 
> Sorry about the confusing word "account", it should be "lockdep/rcu/.."
> 
> > Can you please explain this in more detail? It's not clear which
> > specific case you mean when you say "NMI interrupts an irq", as that
> > could mean a number of distinct scenarios.
> > 
> > AFAICT, if we're in an IRQ handler (with NMIs unmasked), and an NMI
> > causes a new exception we'll do the right thing. So either I'm missing a
> > subtlety or you're describing a different scenario..
> > 
> > Note that the entry code is only trying to distinguish between:
> > 
> > a) This exception is *definitely* an NMI (because regular interrupts
> >    were masked).
> > 
> > b) This exception is *either* and IRQ or an NMI (and this *cannot* be
> >    distinguished until we acknowledge the interrupt), so we treat it as
> >    an IRQ for now.
> > 
> b) is the aim.
> 
> At the entry, enter_el1_irq_or_nmi() -> enter_from_kernel_mode()->rcu_irq_enter()/rcu_irq_enter_check_tick() etc.
> While at irqchip level, gic_handle_irq()->gic_handle_nmi()->nmi_enter(),
> which does not call rcu_irq_enter_check_tick(). So it is not proper to
> "treat it as an IRQ for now"

I'm struggling to understand the problem here. What is "not proper", and
why?

Do you think there's a correctness problem, or that we're doing more
work than necessary? 

If you could give a specific example of a problem, it would really help.

I'm aware that we do more work than strictly necessary when we take a
pNMI from a context with IRQs enabled, but that's how we'd intended this
to work, as it's vastly simpler to manage the state that way. Unless
there's a real problem with that approach I'd prefer to leave it as-is.

Thanks,
Mark.