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: <f692e605-4c89-4290-ac4c-d6115eac56f6@kernel.org>
Date: Mon, 12 May 2025 10:06:23 +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 12. 05. 25, 9:32, Jiri Slaby wrote:
> On 12. 05. 25, 9:25, Jiri Slaby wrote:
>> 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 ^^.
> 
> In fact, this should work around the issue too:

Or not. While the order of initializers is in order as specified, the 
order of side effects wrt to initialization is _unspecified_. So that 
&_t.flags setting can happen any time during initialization.

In sum, _t.flags has to be set out of the initialization for this to be 
working (as you suggested in the code above). Either before or after.

(Setting of .bus can remain in the initializer.)

> --- a/kernel/irq/internals.h
> +++ b/kernel/irq/internals.h
> @@ -178,6 +178,7 @@ static inline class_irqdesc_lock_t 
> class_irqdesc_lock_constructor(unsigned int i
>   {
>          class_irqdesc_lock_t _t = {
>                  .bus    = bus,
> +               .flags  = 0,
>                  .lock   = __irq_get_desc_lock(irq, &_t.flags, bus, check),
>          };
>          return _t;
> 
> 

-- 
js
suse labs