linux-kernel - Re: [PATCH] irq: simplify irq_im_handle

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: <87zfcxtz6t.ffs@tglx>
Date: Mon, 21 Jul 2025 16:07:22 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: Yury Norov <yury.norov@...il.com>, linux-kernel@...r.kernel.org
Cc: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
Subject: Re: [PATCH] irq: simplify irq_im_handle_irq()

Yury!

On Sat, Jul 19 2025 at 17:18, Yury Norov wrote:

'irq:' is not the correct prefix here. See:

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#patch-submission-notes

Also irq_im_handle_irq() is not a known function name.

> From: Yury Norov (NVIDIA) <yury.norov@...il.com>
>
> Hi Thomas,

Since when is a greeting part of the changelog?

> The function calls bitmap_empty() for potentially every bit in
> work_ctx->pending, which makes a simple bitmap traverse O(N^2).
> Fix it by switching to the dedicated for_each_set_bit().
>
> While there, fix using atomic clear_bit() in a context where atomicity
> cannot be guaranteed.

Seriously? See below.

>  static void irq_sim_handle_irq(struct irq_work *work)
>  {
>  	struct irq_sim_work_ctx *work_ctx;
> -	unsigned int offset = 0;
> +	unsigned int offset;
>  	int irqnum;
>  
>  	work_ctx = container_of(work, struct irq_sim_work_ctx, work);
>  
> -	while (!bitmap_empty(work_ctx->pending, work_ctx->irq_count)) {
> -		offset = find_next_bit(work_ctx->pending,
> -				       work_ctx->irq_count, offset);
> -		clear_bit(offset, work_ctx->pending);
> +	for_each_set_bit(offset, work_ctx->pending, work_ctx->irq_count) {
> +		__clear_bit(offset, work_ctx->pending);

This is just wrong.

__clear_bit() can only be used when there is _NO_ concurrency
possible. But this has concurrency:

irq_sim_set_irqchip_state()
...
        assign_bit(hwirq, irq_ctx->work_ctx->pending, state);

That function can be executed on a different CPU concurrently while the
other CPU walks the bitmap and tries to clear a bit. The function
documentation of __clear_bit() has this documented very clearly:

 * Unlike clear_bit(), this function is non-atomic. If it is called on the same
 * region of memory concurrently, the effect may be that only one operation                                                                                    * succeeds.

No?

Thanks,

        tglx