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] [thread-next>] [day] [month] [year] [list]

Message-ID: <be5a4c10-3b69-1c2d-d413-62f79ccc178b@colorfullife.com>
Date:   Fri, 9 Dec 2022 19:47:52 +0100
From:   Manfred Spraul <manfred@...orfullife.com>
To:     "Sverdlin, Alexander" <alexander.sverdlin@...mens.com>,
        "corbet@....net" <corbet@....net>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
        thomas Gleixner <tglx@...utronix.de>,
        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?

Hi Alexander,

On 12/9/22 13:23, Sverdlin, Alexander wrote:
> Dear documentation maintainers,
>
> 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 update for locking.rst would be to switch from spin_lock() to 
mutex_lock().


https://elixir.bootlin.com/linux/latest/source/kernel/irq/manage.c#L126

> /**
> * synchronize_irq - wait for pending IRQ handlers (on other CPUs)
> * @irq: interrupt number to wait for
> *
> * This function waits for any pending IRQ handlers for this interrupt
> * to complete before returning. If you use this function while
> * holding a resource the IRQ handler may need you will deadlock.
> *
> * Can only be called from preemptible code as it might sleep when
> * an interrupt thread is associated to @irq.
> *
> * It optionally makes sure (when the irq chip supports that method)
> * that the interrupt is not pending in any CPU and waiting for
> * service.
> */
> void  synchronize_irq 
> <https://elixir.bootlin.com/linux/latest/C/ident/synchronize_irq>(unsigned  int  irq)
> {
> 	struct  irq_desc <https://elixir.bootlin.com/linux/latest/C/ident/irq_desc>  *desc  =  irq_to_desc 
> <https://elixir.bootlin.com/linux/latest/C/ident/irq_to_desc>(irq);
>
> 	if  (desc)  {
> 		__synchronize_hardirq 
> <https://elixir.bootlin.com/linux/latest/C/ident/__synchronize_hardirq>(desc,  true <https://elixir.bootlin.com/linux/latest/C/ident/true>);
> 		/*
> * We made sure that no hardirq handler is
> * running. Now verify that no threaded handlers are
> * active.
> */
> 		wait_event 
> <https://elixir.bootlin.com/linux/latest/C/ident/wait_event>(desc->wait_for_threads 
> <https://elixir.bootlin.com/linux/latest/C/ident/wait_for_threads>,
> 			!atomic_read 
> <https://elixir.bootlin.com/linux/latest/C/ident/atomic_read>(&desc->threads_active 
> <https://elixir.bootlin.com/linux/latest/C/ident/threads_active>));
> 	}
> }
> EXPORT_SYMBOL 
> <https://elixir.bootlin.com/linux/latest/C/ident/EXPORT_SYMBOL>(synchronize_irq 
> <https://elixir.bootlin.com/linux/latest/C/ident/synchronize_irq>);

https://elixir.bootlin.com/linux/latest/source/kernel/irq/manage.c#L716

> /**
> * disable_irq - disable an irq and wait for completion
> * @irq: Interrupt to disable
> *
> * Disable the selected interrupt line. Enables and Disables are
> * nested.
> * This function waits for any pending IRQ handlers for this interrupt
> * to complete before returning. If you use this function while
> * holding a resource the IRQ handler may need you will deadlock.
> *
> * This function may be called - with care - from IRQ context.
> */
> void  disable_irq 
> <https://elixir.bootlin.com/linux/latest/C/ident/disable_irq>(unsigned  int  irq)
> {
> 	if  (!__disable_irq_nosync 
> <https://elixir.bootlin.com/linux/latest/C/ident/__disable_irq_nosync>(irq))
> 		synchronize_irq 
> <https://elixir.bootlin.com/linux/latest/C/ident/synchronize_irq>(irq);
> }
>
--

     Manfred