linux-kernel - Re: [patch V2 26/45] genirq/chip: Rework irq_set

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: <0a61672b-1782-43ea-bf10-b6088a1fd2fd@kernel.org>
Date: Mon, 12 May 2025 09:25:21 +0200
From: Jiri Slaby <jirislaby@...nel.org>
To: Nathan Chancellor <nathan@...nel.org>, Miguel Ojeda <ojeda@...nel.org>
Cc: linux-kernel@...r.kernel.org, peterz@...radead.org, tglx@...utronix.de
Subject: Re: [patch V2 26/45] genirq/chip: Rework irq_set_handler() variants

On 11. 05. 25, 19:49, Nathan Chancellor wrote:
> On Sun, May 11, 2025 at 07:29:11PM +0200, Miguel Ojeda wrote:
>> On Fri, 09 May 2025 14:22:11 +0100 Nathan Chancellor <nathan@...nel.org> wrote:
>>>
>>> I am investigating some cases where
>>>
>>> WARN(!irqs_disabled(), "Interrupts were enabled early\n");
>>>
>>> in start_kernel() in init/main.c is triggered in certain builds with
>>> clang after patch 23 of this series (very bizarre since the conversion
>>> seems to be correct) and I happened to notice that this conversion seems
>>> to be incorrect? Should this be scoped_irqdesc_get_and_buslock() like
>>> below?
>>
>> Yeah, I am also seeing this in next-20250509 in a LLVM=1 arm64 defconfig + Rust
>> build under QEMU.
> 
> I noticed that the warning was reproducible with just the first patch of
> the series that adds the lock guards and patch 23 but also several other
> individual patches within the series, as I could not just revert patch
> 23 on next-20250509 to fix it. I have no idea why yet because I have not
> had the chance to actually sit down and dig into it but this diff fixes
> every instance of the warning that I saw in my tests... :/ could be a
> compiler bug or just some difference in behavior between compilers.
> 
> diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
> index bd2db6ebb98e..94f463de8f26 100644
> --- a/kernel/irq/internals.h
> +++ b/kernel/irq/internals.h
> @@ -176,10 +176,9 @@ __DEFINE_UNLOCK_GUARD(irqdesc_lock, struct irq_desc,
>   static inline class_irqdesc_lock_t class_irqdesc_lock_constructor(unsigned int irq, bool bus,
>   								  unsigned int check)
>   {
> -	class_irqdesc_lock_t _t = {
> -		.bus	= bus,
> -		.lock	= __irq_get_desc_lock(irq, &_t.flags, bus, check),

I assume the value stored by __irq_get_desc_lock() to &_t.flags is 
overwritten by 0 by the initializer. class_irqdesc_lock_t::flags is 
later than ::lock in the structure, so __irq_get_desc_lock() should be 
called, setting ::flags, then the initializer should set flags to 0.

> -	};
> +	class_irqdesc_lock_t _t = {};
> +	_t.bus	= bus;
> +	_t.lock	= __irq_get_desc_lock(irq, &_t.flags, bus, check);

That's why this works ^^.

>   	return _t;
>   }
>   


-- 
js
suse labs