linux-kernel - Re: [PATCH 2/2] genirq/proc: Refine percpu kstat

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

Message-ID: <87h6f0knau.ffs@tglx>
Date: Wed, 15 May 2024 01:04:41 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: Adrian Huang <adrianhuang0701@...il.com>
Cc: linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org, Jiwei Sun
 <sunjw10@...ovo.com>, Adrian Huang <ahuang12@...ovo.com>
Subject: Re: [PATCH 2/2] genirq/proc: Refine percpu kstat_irqs access logic

On Mon, May 13 2024 at 20:05, Adrian Huang wrote:
> @@ -461,7 +461,7 @@ int show_interrupts(struct seq_file *p, void *v)
>  {
>  	static int prec;
>  
> -	unsigned long flags, any_count = 0;
> +	unsigned long flags, print_irq = 1;

What's wrong with making print_irq boolean?

>  	int i = *(loff_t *) v, j;
>  	struct irqaction *action;
>  	struct irq_desc *desc;
> @@ -488,18 +488,28 @@ int show_interrupts(struct seq_file *p, void *v)
>  	if (!desc || irq_settings_is_hidden(desc))
>  		goto outsparse;
>  
> -	if (desc->kstat_irqs) {
> -		for_each_online_cpu(j)
> -			any_count |= data_race(*per_cpu_ptr(desc->kstat_irqs, j));
> +	if ((!desc->action || irq_desc_is_chained(desc)) && desc->kstat_irqs) {

The condition is wrong. Look how the old code evaluated any_count.

> +		print_irq = 0;
> +		for_each_online_cpu(j) {
> +			if (data_race(*per_cpu_ptr(desc->kstat_irqs, j))) {
> +				print_irq = 1;
> +				break;
> +			}
> +		}

Aside of that this code is just fundamentally wrong in several aspects:

  1) Interrupts which have no action are completely uninteresting as
     there is no real information attached, i.e. it shows that there
     were interrupts on some CPUs, but there is zero information from
     which device they originated.

     Especially with sparse interrupts enabled they are usually gone
     shortly after the last action was removed.

  2) Chained interrupts do not have a count at all as they completely
     evade the core kernel entry points.

So all of this can be avoided and the whole nonsense can be reduced to:

	if (!desc->action || irq_desc_is_chained(desc) || !desc->kstat_irqs)
        	goto outsparse;

which in turn allows to convert this:

> -	for_each_online_cpu(j)
> -		seq_printf(p, "%10u ", desc->kstat_irqs ?
> -					*per_cpu_ptr(desc->kstat_irqs, j) : 0);

into an unconditional:

	for_each_online_cpu(j)
		seq_printf(p, "%10u ", *per_cpu_ptr(desc->kstat_irqs, j));

Thanks,

        tglx