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: <aH5OUChoFx55WnVG@yury>
Date: Mon, 21 Jul 2025 10:27:28 -0400
From: Yury Norov <yury.norov@...il.com>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: linux-kernel@...r.kernel.org,
	Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
Subject: Re: [PATCH] irq: simplify irq_im_handle_irq()

On Mon, Jul 21, 2025 at 04:07:22PM +0200, Thomas Gleixner wrote:
> 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?

find_next_bit() and for_each_bit() cannot be used in concurrent
environment, and having atomic clear_bit() is meaningless here.
Two concurrent processes, if running in parallel, may pick the
same offset, ending up executing the handle_simple_irq() twice.

So, the work_ctx->pending must be local or protected bitmap to make
this all working.

It simply doesn't matter how do you clean the offset - atomically
or not.

I have a series for atomic find_bit() API, not merged though. In
I described it in details there [1].

Or I miss something in the IRQ handling logic?

Thanks,
Yury

[1] https://lists.infradead.org/pipermail/ath10k/2024-June/015900.html