linux-kernel - Re: [PATCH v3] lock/semaphore: Avoid an unnecessary deadlock within up()

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: <20160217104049.GB25010@twins.programming.kicks-ass.net>
Date:	Wed, 17 Feb 2016 11:40:49 +0100
From:	Peter Zijlstra <peterz@...radead.org>
To:	Byungchul Park <byungchul.park@....com>
Cc:	willy@...ux.intel.com, akpm@...ux-foundation.org, mingo@...nel.org,
	linux-kernel@...r.kernel.org, akinobu.mita@...il.com, jack@...e.cz,
	sergey.senozhatsky.work@...il.com, peter@...leysoftware.com
Subject: Re: [PATCH v3] lock/semaphore: Avoid an unnecessary deadlock within
 up()

On Wed, Feb 17, 2016 at 06:11:51PM +0900, Byungchul Park wrote:
> change from v2 to v3
> - the way to solve it in v2 is racy, so I changed the approach entirely.
> - just make semaphore's trylock use spinlock's trylock.
> 
> change from v1 to v2
> - remove unnecessary overhead by the redundant spin(un)lock.
> 
> Since I faced a infinite recursive printk() bug, I've tried to propose
> patches the title of which is "lib/spinlock_debug.c: prevent a recursive
> cycle in the debug code". But I noticed the root problem cannot be fixed
> by that, through some discussion thanks to Sergey and Peter. So I focused
> on preventing the deadlock.
> 
> -----8<-----
> From 43e029ca920890ac644e30d873be69cf5d01efdb Mon Sep 17 00:00:00 2001
> From: Byungchul Park <byungchul.park@....com>
> Date: Wed, 17 Feb 2016 17:22:18 +0900
> Subject: [PATCH v3] lock/semaphore: Avoid an unnecessary deadlock within up()
> 
> One of semaphore acquisition functions, down_trylock() is implemented
> using raw_spin_lock_irqsave(&sem->lock) even though it's enough to use
> raw_spin_trylock_irqsave(). Furthermore, using raw_spin_lock_irqsave()
> can cause a unnecessary deadlock as described below. Just make it use
> the spinlock trylock to implement the semaphore trylock so that we can
> avoid the unnecessary deadlock happened.
> 
> The scenario the bad thing can happen is,
> 
> printk
>   console_trylock
>   console_unlock
>     up_console_sem
>       up
>         raw_spin_lock_irqsave(&sem->lock, flags)
>         __up
>           wake_up_process
>             try_to_wake_up
>               raw_spin_lock_irqsave(&p->pi_lock)
>                 __spin_lock_debug
>                   spin_dump
>                     printk
>                       console_trylock
>                         raw_spin_lock_irqsave(&sem->lock, flags)
>                         >>> DEADLOCK <<<
> 
> Signed-off-by: Byungchul Park <byungchul.park@....com>

Mucking with the semaphore implementation just because printk() is
terminally broken shite really doesn't fly.