linux-kernel - Re: [PATCH v6 2/4] genirq: Provide NMI management for percpu

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

Date:   Wed, 13 Feb 2019 09:57:17 +0000
From:   Julien Thierry <julien.thierry@....com>
To:     "liwei (GF)" <liwei391@...wei.com>, linux-kernel@...r.kernel.org
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>,
        Marc Zyngier <marc.zyngier@....com>,
        Ingo Molnar <mingo@...nel.org>
Subject: Re: [PATCH v6 2/4] genirq: Provide NMI management for percpu_devid
 interrupts

Hi Li Wei,

[Adding back Cc that were dropped]

On 12/02/2019 09:51, liwei (GF) wrote:
> Hi Julien,
> 
> On 2019/1/31 22:53, Julien Thierry Wrote:
>> Add support for percpu_devid interrupts treated as NMIs.
>>
>> Percpu_devid NMIs need to be setup/torn down on each CPU they target.
>>
>> The same restrictions as for global NMIs still apply for percpu_devid NMIs.
>>
>> Signed-off-by: Julien Thierry <julien.thierry@....com>
>> Cc: Thomas Gleixner <tglx@...utronix.de>
>> Cc: Peter Zijlstra <peterz@...radead.org>
>> Cc: Ingo Molnar <mingo@...nel.org>
>> Cc: Marc Zyngier <marc.zyngier@....com>
>> ---
>>  include/linux/interrupt.h |   9 +++
>>  kernel/irq/manage.c       | 177 ++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 186 insertions(+)
>>
> (SNIP)
>>  
>>  /**
>> + *	request_percpu_nmi - allocate a percpu interrupt line for NMI delivery
>> + *	@irq: Interrupt line to allocate
>> + *	@handler: Function to be called when the IRQ occurs.
>> + *	@name: An ascii name for the claiming device
>> + *	@dev_id: A percpu cookie passed back to the handler function
>> + *
>> + *	This call allocates interrupt resources for a per CPU NMI. Per CPU NMIs
>> + *	have to be setup on each CPU by calling ready_percpu_nmi() before being
> ready_percpu_nmi need to be renamed here.
> 

Thanks for spotting this. I'll send a fix for next.

>> + *	enabled on the same CPU by using enable_percpu_nmi().
>> + *
>> + *	Dev_id must be globally unique. It is a per-cpu variable, and
>> + *	the handler gets called with the interrupted CPU's instance of
>> + *	that variable.
>> + *
>> + *	Interrupt lines requested for NMI delivering should have auto enabling
>> + *	setting disabled.
>> + *
>> + *	If the interrupt line cannot be used to deliver NMIs, function
>> + *	will fail returning a negative value.
>> + */
>> +int request_percpu_nmi(unsigned int irq, irq_handler_t handler,
>> +		       const char *name, void __percpu *dev_id)
>> +{
>> +	struct irqaction *action;
>> +	struct irq_desc *desc;
>> +	unsigned long flags;
>> +	int retval;
>> +
>> +	if (!handler)
>> +		return -EINVAL;
>> +
>> +	desc = irq_to_desc(irq);
>> +
> and i have an other question, i found that kstat_incr_irqs_this_cpu() was omitted in
> handle_fasteoi_nmi() and handle_percpu_devid_fasteoi_nmi(), are there some worrys here?
> It is a little odd since we can get NMI counters on the x86 machine in /proc/interrputs.

I don't think x86 uses the IRQ framework for NMIs and probably have
their own thing to count NMIs.

The issue is that we cannot take the desc lock so I'm not sure it would
be safe to increment the desc->tot_count in NMI context (although
technically NMI IRQ lines cannot be shared, so I think it should be fine
for non-percpu_nmis). The other concern is incrementing the percpu
kstat.irqs_sum, which I guess is a definite *no* in an NMI context as we
could have interrupted and interrupt trying to increment that same counter.

So we might be able to establish some kstat_incr_nmi to update some of
the counters but it might require a bit more reflexion on the matter, so
I'd like to avoid adding that as a "quick fix" in next which might end
up breaking things.

Thanks,

-- 
Julien Thierry