linux-kernel - Re: Invalid locking pattern in Documentation/kernel-hacking/locking.rst?

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]

Date:   Mon, 12 Dec 2022 15:43:44 +0100
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Manfred Spraul <manfred@...orfullife.com>,
        "Sverdlin, Alexander" <alexander.sverdlin@...mens.com>,
        "corbet@....net" <corbet@....net>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
        Ingo Molnar <mingo@...nel.org>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        1vier1@....de
Subject: Re: Invalid locking pattern in
 Documentation/kernel-hacking/locking.rst?

On Fri, Dec 09 2022 at 19:47, Manfred Spraul wrote:
> On 12/9/22 13:23, Sverdlin, Alexander wrote:
>> the latest version of locking.rst contains the following (since 2005):
>>
>> "Manfred Spraul points out that you can still do this, even if the data
>> is very occasionally accessed in user context or softirqs/tasklets. The
>> irq handler doesn't use a lock, and all other accesses are done as so::
>>
>>          spin_lock(&lock);
>>          disable_irq(irq);
>> "
>>
>> Isn't it "sleeping in atomic" actually because of the sleeping
>> disable_irq()?
>
> Good catch!
>
> The documentation of disable_irq() claims that the function is safe to 
> be called from IRQ context (for careful callers)
>
> But it calls synchronize_irq(). And synchronize_irq() claims that the 
> function can be called only from preemptible code.
>
> The change was in 2009:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v6.1-rc8&id=3aa551c9b4c40018f0e261a178e3d25478dc04a9 
>
>
> @Thomas/@...o: What do we want?
>
> Declare disable_irq()&synchronize_irq() as safe from irq context only if 
> no threaded irq handlers are used?
>
> Or declare both function as preemptible context only?

The latter.

The comment for disable_irq() needs to go away too:

 "This function may be called - with care - from IRQ context."

Obviously it can't be called from the interrupt context which it
tries to disable as it will live-lock on the "in progress" flag.

So that leaves the option to call it from some unrelated interrupt
context which does not make much sense. In fact, back in the days when
this comment was added it was still allowed to reenable interrupts in
the interrupt handler, which obviously opens the window for some other
interrupt to come in which then tries to disable the one it just
interrupted. Not an issue anymore, but the synchronize_irq() change to
handle threaded interrupts made it more or less impossible.

Thanks,

        tglx