linux-kernel - Re: [RFC] workqueue: Fix irq inversion deadlock in manage

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: <20171008190109.GX3301751@devbig577.frc2.facebook.com>
Date:   Sun, 8 Oct 2017 12:03:47 -0700
From:   Tejun Heo <tj@...nel.org>
To:     Boqun Feng <boqun.feng@...il.com>
Cc:     linux-kernel@...r.kernel.org, Josef Bacik <josef@...icpanda.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Lai Jiangshan <jiangshanlai@...il.com>
Subject: Re: [RFC] workqueue: Fix irq inversion deadlock in manage_workers()

Hello, Boqun.

On Sun, Oct 08, 2017 at 05:02:23PM +0800, Boqun Feng wrote:
> Josef reported a HARDIRQ-safe -> HARDIRQ-unsafe lock order detected by
> lockdep:
> 
> | [ 1270.472259] WARNING: HARDIRQ-safe -> HARDIRQ-unsafe lock order detected
> | [ 1270.472783] 4.14.0-rc1-xfstests-12888-g76833e8 #110 Not tainted
> | [ 1270.473240] -----------------------------------------------------
> | [ 1270.473710] kworker/u5:2/5157 [HC0[0]:SC0[0]:HE0:SE1] is trying to acquire:
> | [ 1270.474239]  (&(&lock->wait_lock)->rlock){+.+.}, at: [<ffffffff8da253d2>] __mutex_unlock_slowpath+0xa2/0x280
> | [ 1270.474994]
> | [ 1270.474994] and this task is already holding:
> | [ 1270.475440]  (&pool->lock/1){-.-.}, at: [<ffffffff8d2992f6>] worker_thread+0x366/0x3c0
> | [ 1270.476046] which would create a new lock dependency:
> | [ 1270.476436]  (&pool->lock/1){-.-.} -> (&(&lock->wait_lock)->rlock){+.+.}
> | [ 1270.476949]
> | [ 1270.476949] but this new dependency connects a HARDIRQ-irq-safe lock:
> | [ 1270.477553]  (&pool->lock/1){-.-.}
> ...
> | [ 1270.488900] to a HARDIRQ-irq-unsafe lock:
> | [ 1270.489327]  (&(&lock->wait_lock)->rlock){+.+.}
> ...
> | [ 1270.494735]  Possible interrupt unsafe locking scenario:
> | [ 1270.494735]
> | [ 1270.495250]        CPU0                    CPU1
> | [ 1270.495600]        ----                    ----
> | [ 1270.495947]   lock(&(&lock->wait_lock)->rlock);
> | [ 1270.496295]                                local_irq_disable();
> | [ 1270.496753]                                lock(&pool->lock/1);
> | [ 1270.497205]                                lock(&(&lock->wait_lock)->rlock);
> | [ 1270.497744]   <Interrupt>
> | [ 1270.497948]     lock(&pool->lock/1);
> 
> , which will cause a irq inversion deadlock if the above lock scenario
> happens.
> 
> The root cause of this safe -> unsafe lock order is the
> mutex_unlock(pool::manager_arb) in manage_workers() with pool::lock

So, if I'm not mistaken, this is a regression caused by b9c16a0e1f73
("locking/mutex: Fix lockdep_assert_held() fail") which seems to
replace irqsave operations inside mutex to unconditional irq ones.

I suppose it's a requirement we can add but that needs to be an
explicit change with backing rationales.

Thanks.

-- 
tejun