Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1307793 > unrolled thread

timers: HARDIRQ-safe -> HARDIRQ-unsafe lock order detected

Started bySasha Levin <sasha.levin@oracle.com>
First post2016-01-12 21:10 +0100
Last post2016-01-15 02:50 +0100
Articles 9 — 4 participants

Back to article view | Back to linux.kernel

Contents

  timers: HARDIRQ-safe -> HARDIRQ-unsafe lock order detected Sasha Levin <sasha.levin@oracle.com> - 2016-01-12 21:10 +0100
    Re: timers: HARDIRQ-safe -> HARDIRQ-unsafe lock order detected Peter Zijlstra <peterz@infradead.org> - 2016-01-12 21:20 +0100
      Re: timers: HARDIRQ-safe -> HARDIRQ-unsafe lock order detected "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-01-12 22:00 +0100
    Re: timers: HARDIRQ-safe -> HARDIRQ-unsafe lock order detected Thomas Gleixner <tglx@linutronix.de> - 2016-01-13 10:10 +0100
      Re: timers: HARDIRQ-safe -> HARDIRQ-unsafe lock order detected "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-01-13 17:30 +0100
        Re: timers: HARDIRQ-safe -> HARDIRQ-unsafe lock order detected Thomas Gleixner <tglx@linutronix.de> - 2016-01-14 18:50 +0100
          Re: timers: HARDIRQ-safe -> HARDIRQ-unsafe lock order detected "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-01-14 20:40 +0100
            Re: timers: HARDIRQ-safe -> HARDIRQ-unsafe lock order detected Thomas Gleixner <tglx@linutronix.de> - 2016-01-14 20:50 +0100
              Re: timers: HARDIRQ-safe -> HARDIRQ-unsafe lock order detected "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-01-15 02:50 +0100

#1307793 — timers: HARDIRQ-safe -> HARDIRQ-unsafe lock order detected

FromSasha Levin <sasha.levin@oracle.com>
Date2016-01-12 21:10 +0100
Subjecttimers: HARDIRQ-safe -> HARDIRQ-unsafe lock order detected
Message-ID<qQdqq-5Tj-9@gated-at.bofh.it>
Hi all,

While fuzzing with trinity inside a KVM tools guest, running the latest -next
kernel, I've hit the following lockdep warning:

[ 3408.411602] ======================================================

[ 3408.418607] [ INFO: HARDIRQ-safe -> HARDIRQ-unsafe lock order detected ]

[ 3408.419322] 4.4.0-rc8-next-20160111-sasha-00024-g376a9c2-dirty #2782 Not tainted

[ 3408.420171] ------------------------------------------------------

[ 3408.420984] trinity-c65/30907 [HC0[0]:SC0[0]:HE0:SE1] is trying to acquire:

[ 3408.421869] (&lock->wait_lock){+.+...}, at: rt_mutex_slowunlock (kernel/locking/rtmutex.c:1266)
[ 3408.423182]

[ 3408.423182] and this task is already holding:

[ 3408.423916] (&(&new_timer->it_lock)->rlock){-.-...}, at: __lock_timer (kernel/time/posix-timers.c:707)
[ 3408.425281] which would create a new lock dependency:

[ 3408.425942]  (&(&new_timer->it_lock)->rlock){-.-...} -> (&lock->wait_lock){+.+...}

[ 3408.427118]

[ 3408.427118] but this new dependency connects a HARDIRQ-irq-safe lock:

[ 3408.428093]  (&(&new_timer->it_lock)->rlock){-.-...}

... which became HARDIRQ-irq-safe at:

[ 3408.429232] __lock_acquire (kernel/locking/lockdep.c:2796 kernel/locking/lockdep.c:3162)
[ 3408.430075] lock_acquire (./arch/x86/include/asm/current.h:14 kernel/locking/lockdep.c:3587)
[ 3408.430814] _raw_spin_lock (include/linux/spinlock_api_smp.h:145 kernel/locking/spinlock.c:151)
[ 3408.431584] run_posix_cpu_timers (include/linux/list.h:156 kernel/time/posix-cpu-timers.c:1231)
[ 3408.437683] update_process_times (kernel/time/timer.c:1427)
[ 3408.438528] tick_sched_handle (kernel/time/tick-sched.c:152)
[ 3408.439305] tick_sched_timer (kernel/time/tick-sched.c:1089)
[ 3408.440086] __hrtimer_run_queues (kernel/time/hrtimer.c:1233 kernel/time/hrtimer.c:1295)
[ 3408.446607] hrtimer_interrupt (kernel/time/hrtimer.c:1332)
[ 3408.447427] smp_trace_apic_timer_interrupt (./arch/x86/include/asm/apic.h:659 arch/x86/kernel/apic/apic.c:953)
[ 3408.448406] smp_apic_timer_interrupt (arch/x86/kernel/apic/apic.c:956)
[ 3408.460706] apic_timer_interrupt (arch/x86/entry/entry_64.S:687)
[ 3408.461517] _raw_spin_lock (include/linux/spinlock_api_smp.h:145 kernel/locking/spinlock.c:151)
[ 3408.462259] sync_inodes_sb (fs/fs-writeback.c:2133 fs/fs-writeback.c:2292)
[ 3408.463042] sync_inodes_one_sb (fs/sync.c:74)
[ 3408.463826] iterate_supers (fs/super.c:535)
[ 3408.464583] sys_sync (fs/sync.c:113)
[ 3408.465239] entry_SYSCALL_64_fastpath (arch/x86/entry/entry_64.S:186)
[ 3408.466074]

[ 3408.466074] to a HARDIRQ-irq-unsafe lock:

[ 3408.466732]  (&lock->wait_lock){+.+...}

... which became HARDIRQ-irq-unsafe at:

[ 3408.467701] ... __lock_acquire (kernel/locking/lockdep.c:2813 kernel/locking/lockdep.c:3162)
[ 3408.468551] lock_acquire (./arch/x86/include/asm/current.h:14 kernel/locking/lockdep.c:3587)
[ 3408.469240] _raw_spin_lock (include/linux/spinlock_api_smp.h:145 kernel/locking/spinlock.c:151)
[ 3408.469987] rt_mutex_slowlock (kernel/locking/rtmutex.c:1184)
[ 3408.470722] rt_mutex_lock (kernel/locking/rtmutex.c:1402)
[ 3408.471416] rcu_boost_kthread (kernel/rcu/tree_plugin.h:1033 kernel/rcu/tree_plugin.h:1055)
[ 3408.472253] kthread (kernel/kthread.c:209)
[ 3408.472937] ret_from_fork (arch/x86/entry/entry_64.S:469)
[ 3408.473642]

[ 3408.473642] other info that might help us debug this:

[ 3408.473642]

[ 3408.474461]  Possible interrupt unsafe locking scenario:

[ 3408.474461]

[ 3408.475239]        CPU0                    CPU1

[ 3408.475809]        ----                    ----

[ 3408.476380]   lock(&lock->wait_lock);

[ 3408.476925]                                local_irq_disable();

[ 3408.477640]                                lock(&(&new_timer->it_lock)->rlock);

[ 3408.478607]                                lock(&lock->wait_lock);

[ 3408.479445]   <Interrupt>

[ 3408.479796]     lock(&(&new_timer->it_lock)->rlock);

[ 3408.480504]

[ 3408.480504]  *** DEADLOCK ***

[ 3408.480504]

[ 3408.481270] 2 locks held by trinity-c65/30907:

[ 3408.481826] #0: (rcu_read_lock){......}, at: __lock_timer (include/linux/rcupdate.h:872 kernel/time/posix-timers.c:704)
[ 3408.483007] #1: (&(&new_timer->it_lock)->rlock){-.-...}, at: __lock_timer (kernel/time/posix-timers.c:707)
[ 3408.484369]

the dependencies between HARDIRQ-irq-safe lock and the holding lock:

[ 3408.485317] -> (&(&new_timer->it_lock)->rlock){-.-...} ops: 3448 {

[ 3408.486264]    IN-HARDIRQ-W at:

[ 3408.486707] __lock_acquire (kernel/locking/lockdep.c:2796 kernel/locking/lockdep.c:3162)
[ 3408.487670] lock_acquire (./arch/x86/include/asm/current.h:14 kernel/locking/lockdep.c:3587)
[ 3408.504632] _raw_spin_lock (include/linux/spinlock_api_smp.h:145 kernel/locking/spinlock.c:151)
[ 3408.505511] run_posix_cpu_timers (include/linux/list.h:156 kernel/time/posix-cpu-timers.c:1231)
[ 3408.506496] update_process_times (kernel/time/timer.c:1427)
[ 3408.507473] tick_sched_handle (kernel/time/tick-sched.c:152)
[ 3408.508447] tick_sched_timer (kernel/time/tick-sched.c:1089)
[ 3408.509366] __hrtimer_run_queues (kernel/time/hrtimer.c:1233 kernel/time/hrtimer.c:1295)
[ 3408.510356] hrtimer_interrupt (kernel/time/hrtimer.c:1332)
[ 3408.511308] smp_trace_apic_timer_interrupt (./arch/x86/include/asm/apic.h:659 arch/x86/kernel/apic/apic.c:953)
[ 3408.512386] smp_apic_timer_interrupt (arch/x86/kernel/apic/apic.c:956)
[ 3408.513380] apic_timer_interrupt (arch/x86/entry/entry_64.S:687)
[ 3408.514351] _raw_spin_lock (include/linux/spinlock_api_smp.h:145 kernel/locking/spinlock.c:151)
[ 3408.515112] sync_inodes_sb (fs/fs-writeback.c:2133 fs/fs-writeback.c:2292)
[ 3408.528093] sync_inodes_one_sb (fs/sync.c:74)
[ 3408.529101] iterate_supers (fs/super.c:535)
[ 3408.530052] sys_sync (fs/sync.c:113)
[ 3408.530926] entry_SYSCALL_64_fastpath (arch/x86/entry/entry_64.S:186)
[ 3408.531988]    IN-SOFTIRQ-W at:

[ 3408.532442] __lock_acquire (kernel/locking/lockdep.c:2799 kernel/locking/lockdep.c:3162)
[ 3408.533415] lock_acquire (./arch/x86/include/asm/current.h:14 kernel/locking/lockdep.c:3587)
[ 3408.534345] _raw_spin_lock (include/linux/spinlock_api_smp.h:145 kernel/locking/spinlock.c:151)
[ 3408.535239] run_posix_cpu_timers (include/linux/list.h:156 kernel/time/posix-cpu-timers.c:1231)
[ 3408.552300] update_process_times (kernel/time/timer.c:1427)
[ 3408.553305] tick_sched_handle (kernel/time/tick-sched.c:152)
[ 3408.554283] tick_sched_timer (kernel/time/tick-sched.c:1089)
[ 3408.555241] __hrtimer_run_queues (kernel/time/hrtimer.c:1233 kernel/time/hrtimer.c:1295)
[ 3408.556226] hrtimer_interrupt (kernel/time/hrtimer.c:1332)
[ 3408.557101] smp_trace_apic_timer_interrupt (./arch/x86/include/asm/apic.h:659 arch/x86/kernel/apic/apic.c:953)
[ 3408.558131] smp_apic_timer_interrupt (arch/x86/kernel/apic/apic.c:956)
[ 3408.559122] apic_timer_interrupt (arch/x86/entry/entry_64.S:687)
[ 3408.560128] irq_exit (kernel/softirq.c:350 kernel/softirq.c:391)
[ 3408.561008] smp_trace_apic_timer_interrupt (./arch/x86/include/asm/irq_regs.h:26 arch/x86/kernel/apic/apic.c:955)
[ 3408.562123] smp_apic_timer_interrupt (arch/x86/kernel/apic/apic.c:956)
[ 3408.563153] apic_timer_interrupt (arch/x86/entry/entry_64.S:687)
[ 3408.566483]    INITIAL USE at:

[ 3408.566917] __lock_acquire (kernel/locking/lockdep.c:3166)
[ 3408.567903] lock_acquire (./arch/x86/include/asm/current.h:14 kernel/locking/lockdep.c:3587)
[ 3408.568826] _raw_spin_lock_irqsave (include/linux/spinlock_api_smp.h:119 kernel/locking/spinlock.c:159)
[ 3408.569824] exit_itimers (kernel/time/posix-timers.c:936 kernel/time/posix-timers.c:983 kernel/time/posix-timers.c:1008)
[ 3408.570725] do_exit (kernel/exit.c:724)
[ 3408.571606] do_group_exit (kernel/exit.c:862)
[ 3408.572539] get_signal (kernel/signal.c:2327)
[ 3408.573458] do_signal (arch/x86/kernel/signal.c:781)
[ 3408.574357] exit_to_usermode_loop (arch/x86/entry/common.c:249)
[ 3408.575335] syscall_return_slowpath (./arch/x86/include/asm/jump_label.h:35 include/linux/context_tracking_state.h:30 include/linux/context_tracking.h:24 arch/x86/entry/common.c:284 arch/x86/entry/common.c:344)
[ 3408.576356] int_ret_from_sys_call (arch/x86/entry/entry_64.S:282)
[ 3408.577326]  }

[ 3408.577574] ... key at: __key.35766 (??:?)
[ 3408.578477]  ... acquired at:

[ 3408.578863] check_irq_usage (kernel/locking/lockdep.c:1649)
[ 3408.579640] __lock_acquire (kernel/locking/lockdep_states.h:7 kernel/locking/lockdep.c:1857 kernel/locking/lockdep.c:1958 kernel/locking/lockdep.c:2144 kernel/locking/lockdep.c:3206)
[ 3408.596505] lock_acquire (./arch/x86/include/asm/current.h:14 kernel/locking/lockdep.c:3587)
[ 3408.597191] _raw_spin_lock (include/linux/spinlock_api_smp.h:145 kernel/locking/spinlock.c:151)
[ 3408.597965] rt_mutex_slowunlock (kernel/locking/rtmutex.c:1266)
[ 3408.598720] rt_mutex_unlock (kernel/locking/rtmutex.c:1384 kernel/locking/rtmutex.c:1486)
[ 3408.599499] rcu_read_unlock_special (kernel/rcu/tree_plugin.h:503)
[ 3408.603731] __rcu_read_unlock (kernel/rcu/update.c:223)
[ 3408.604452] __lock_timer (include/linux/rcupdate.h:495 include/linux/rcupdate.h:930 kernel/time/posix-timers.c:709)
[ 3408.605221] SyS_timer_settime (kernel/time/posix-timers.c:903 kernel/time/posix-timers.c:881)
[ 3408.605982] entry_SYSCALL_64_fastpath (arch/x86/entry/entry_64.S:186)
[ 3408.606791]

[ 3408.607008]

the dependencies between the lock to be acquired and HARDIRQ-irq-unsafe lock:

[ 3408.608038] -> (&lock->wait_lock){+.+...} ops: 134 {

[ 3408.608843]    HARDIRQ-ON-W at:

[ 3408.609209] __lock_acquire (kernel/locking/lockdep.c:2813 kernel/locking/lockdep.c:3162)
[ 3408.610199] lock_acquire (./arch/x86/include/asm/current.h:14 kernel/locking/lockdep.c:3587)
[ 3408.611069] _raw_spin_lock (include/linux/spinlock_api_smp.h:145 kernel/locking/spinlock.c:151)
[ 3408.611996] rt_mutex_slowlock (kernel/locking/rtmutex.c:1184)
[ 3408.612972] rt_mutex_lock (kernel/locking/rtmutex.c:1402)
[ 3408.613856] rcu_boost_kthread (kernel/rcu/tree_plugin.h:1033 kernel/rcu/tree_plugin.h:1055)
[ 3408.614733] kthread (kernel/kthread.c:209)
[ 3408.615599] ret_from_fork (arch/x86/entry/entry_64.S:469)
[ 3408.628604]    SOFTIRQ-ON-W at:

[ 3408.629055] __lock_acquire (kernel/locking/lockdep.c:2817 kernel/locking/lockdep.c:3162)
[ 3408.630046] lock_acquire (./arch/x86/include/asm/current.h:14 kernel/locking/lockdep.c:3587)
[ 3408.630992] _raw_spin_lock (include/linux/spinlock_api_smp.h:145 kernel/locking/spinlock.c:151)
[ 3408.638480] rt_mutex_slowlock (kernel/locking/rtmutex.c:1184)
[ 3408.640811] rt_mutex_lock (kernel/locking/rtmutex.c:1402)
[ 3408.644207] rcu_boost_kthread (kernel/rcu/tree_plugin.h:1033 kernel/rcu/tree_plugin.h:1055)
[ 3408.647236] kthread (kernel/kthread.c:209)
[ 3408.650140] ret_from_fork (arch/x86/entry/entry_64.S:469)
[ 3408.653122]    INITIAL USE at:

[ 3408.654723] __lock_acquire (kernel/locking/lockdep.c:3166)
[ 3408.656792] lock_acquire (./arch/x86/include/asm/current.h:14 kernel/locking/lockdep.c:3587)
[ 3408.658136] _raw_spin_lock (include/linux/spinlock_api_smp.h:145 kernel/locking/spinlock.c:151)
[ 3408.659612] rt_mutex_slowlock (kernel/locking/rtmutex.c:1184)
[ 3408.660998] rt_mutex_lock (kernel/locking/rtmutex.c:1402)
[ 3408.663768] rcu_boost_kthread (kernel/rcu/tree_plugin.h:1033 kernel/rcu/tree_plugin.h:1055)
[ 3408.672030] kthread (kernel/kthread.c:209)
[ 3408.675053] ret_from_fork (arch/x86/entry/entry_64.S:469)
[ 3408.678075]  }

[ 3408.678996] ... key at: __key.19737 (??:?)
[ 3408.681194]  ... acquired at:

[ 3408.682967] check_irq_usage (kernel/locking/lockdep.c:1649)
[ 3408.684098] __lock_acquire (kernel/locking/lockdep_states.h:7 kernel/locking/lockdep.c:1857 kernel/locking/lockdep.c:1958 kernel/locking/lockdep.c:2144 kernel/locking/lockdep.c:3206)
[ 3408.685496] lock_acquire (./arch/x86/include/asm/current.h:14 kernel/locking/lockdep.c:3587)
[ 3408.686597] _raw_spin_lock (include/linux/spinlock_api_smp.h:145 kernel/locking/spinlock.c:151)
[ 3408.687693] rt_mutex_slowunlock (kernel/locking/rtmutex.c:1266)
[ 3408.688916] rt_mutex_unlock (kernel/locking/rtmutex.c:1384 kernel/locking/rtmutex.c:1486)
[ 3408.690042] rcu_read_unlock_special (kernel/rcu/tree_plugin.h:503)
[ 3408.691451] __rcu_read_unlock (kernel/rcu/update.c:223)
[ 3408.692629] __lock_timer (include/linux/rcupdate.h:495 include/linux/rcupdate.h:930 kernel/time/posix-timers.c:709)
[ 3408.693743] SyS_timer_settime (kernel/time/posix-timers.c:903 kernel/time/posix-timers.c:881)
[ 3408.694978] entry_SYSCALL_64_fastpath (arch/x86/entry/entry_64.S:186)
[ 3408.696162]

[ 3408.696629]

[ 3408.696629] stack backtrace:

[ 3408.697558] CPU: 0 PID: 30907 Comm: trinity-c65 Not tainted 4.4.0-rc8-next-20160111-sasha-00024-g376a9c2-dirty #2782

[ 3408.699544]  1ffff100188b2ecd 000000003efba18a ffff8800c45976e8 ffffffffa401ab02

[ 3408.701068]  0000000041b58ab3 ffffffffaf1b73b8 ffffffffa401aa37 ffff8800c45976e8

[ 3408.702589]  ffffffffa245a127 000000003efba18a 000000005046ad48 0000000000000001

[ 3408.703754] Call Trace:

[ 3408.704185] dump_stack (lib/dump_stack.c:52)
[ 3408.707084] check_usage (kernel/locking/lockdep.c:1561)
[ 3408.714764] check_irq_usage (kernel/locking/lockdep.c:1649)
[ 3408.715747] __lock_acquire (kernel/locking/lockdep_states.h:7 kernel/locking/lockdep.c:1857 kernel/locking/lockdep.c:1958 kernel/locking/lockdep.c:2144 kernel/locking/lockdep.c:3206)
[ 3408.722807] lock_acquire (./arch/x86/include/asm/current.h:14 kernel/locking/lockdep.c:3587)
[ 3408.726723] _raw_spin_lock (include/linux/spinlock_api_smp.h:145 kernel/locking/spinlock.c:151)
[ 3408.728982] rt_mutex_slowunlock (kernel/locking/rtmutex.c:1266)
[ 3408.730066] rt_mutex_unlock (kernel/locking/rtmutex.c:1384 kernel/locking/rtmutex.c:1486)
[ 3408.733192] rcu_read_unlock_special (kernel/rcu/tree_plugin.h:503)
[ 3408.735155] __rcu_read_unlock (kernel/rcu/update.c:223)
[ 3408.736090] __lock_timer (include/linux/rcupdate.h:495 include/linux/rcupdate.h:930 kernel/time/posix-timers.c:709)
[ 3408.737947] SyS_timer_settime (kernel/time/posix-timers.c:903 kernel/time/posix-timers.c:881)
[ 3408.744580] entry_SYSCALL_64_fastpath (arch/x86/entry/entry_64.S:186)

[toc] | [next] | [standalone]

#1307806

FromPeter Zijlstra <peterz@infradead.org>
Date2016-01-12 21:20 +0100
Message-ID<qQdA6-5WY-25@gated-at.bofh.it>
In reply to#1307793
On Tue, Jan 12, 2016 at 03:03:27PM -0500, Sasha Levin wrote:
> [ 3408.703754] Call Trace:

> [ 3408.733192] rcu_read_unlock_special (kernel/rcu/tree_plugin.h:503)
> [ 3408.735155] __rcu_read_unlock (kernel/rcu/update.c:223)
> [ 3408.736090] __lock_timer (include/linux/rcupdate.h:495 include/linux/rcupdate.h:930 kernel/time/posix-timers.c:709)

I'm thinking this is one of those magic preemptible RCU bits..

---
 kernel/time/posix-timers.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
index 31d11ac9fa47..09e28733e725 100644
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -701,17 +701,25 @@ static struct k_itimer *__lock_timer(timer_t timer_id, unsigned long *flags)
 	if ((unsigned long long)timer_id > INT_MAX)
 		return NULL;
 
+	/*
+	 * One of the few rules of preemptible RCU is that one cannot do
+	 * rcu_read_unlock() while holding a scheduler (or nested) lock when
+	 * part of the read side critical section was irqs-enabled -- see
+	 * rcu_read_unlock_special().
+	 */
+	local_irq_safe(*flags);
 	rcu_read_lock();
 	timr = posix_timer_by_id(timer_id);
 	if (timr) {
-		spin_lock_irqsave(&timr->it_lock, *flags);
+		spin_lock(&timr->it_lock);
 		if (timr->it_signal == current->signal) {
 			rcu_read_unlock();
 			return timr;
 		}
-		spin_unlock_irqrestore(&timr->it_lock, *flags);
+		spin_unlock(&timr->it_lock);
 	}
 	rcu_read_unlock();
+	local_irq_restore(*flags);
 
 	return NULL;
 }

[toc] | [prev] | [next] | [standalone]

#1307823

From"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Date2016-01-12 22:00 +0100
Message-ID<qQecN-6d2-1@gated-at.bofh.it>
In reply to#1307806
On Tue, Jan 12, 2016 at 09:18:56PM +0100, Peter Zijlstra wrote:
> On Tue, Jan 12, 2016 at 03:03:27PM -0500, Sasha Levin wrote:
> > [ 3408.703754] Call Trace:
> 
> > [ 3408.733192] rcu_read_unlock_special (kernel/rcu/tree_plugin.h:503)
> > [ 3408.735155] __rcu_read_unlock (kernel/rcu/update.c:223)
> > [ 3408.736090] __lock_timer (include/linux/rcupdate.h:495 include/linux/rcupdate.h:930 kernel/time/posix-timers.c:709)
> 
> I'm thinking this is one of those magic preemptible RCU bits..

Hmmm...  Looking back at Sasha's original email, RCU doesn't have much
choice about making ->wait_lock HARDIRQ-irq-unsafe, since it acquires
it via a call to rt_mutex_lock(), which cannot be invoked with irqs
disabled.  In fact, it seems a bit odd to acquire something named
->wait_lock with irqs disabled.

That said...

> ---
>  kernel/time/posix-timers.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
> index 31d11ac9fa47..09e28733e725 100644
> --- a/kernel/time/posix-timers.c
> +++ b/kernel/time/posix-timers.c
> @@ -701,17 +701,25 @@ static struct k_itimer *__lock_timer(timer_t timer_id, unsigned long *flags)
>  	if ((unsigned long long)timer_id > INT_MAX)
>  		return NULL;
> 
> +	/*
> +	 * One of the few rules of preemptible RCU is that one cannot do
> +	 * rcu_read_unlock() while holding a scheduler (or nested) lock when
> +	 * part of the read side critical section was irqs-enabled -- see
> +	 * rcu_read_unlock_special().
> +	 */
> +	local_irq_safe(*flags);
>  	rcu_read_lock();
>  	timr = posix_timer_by_id(timer_id);
>  	if (timr) {
> -		spin_lock_irqsave(&timr->it_lock, *flags);
> +		spin_lock(&timr->it_lock);
>  		if (timr->it_signal == current->signal) {
>  			rcu_read_unlock();

If ->it_lock is ever acquired while one of the rq or pi locks was held,
Peter's patch is needed.

It is just that I am not seeing what I would expect to see in Sasha's
lockdep splat if that were the case.

							Thanx, Paul

>  			return timr;
>  		}
> -		spin_unlock_irqrestore(&timr->it_lock, *flags);
> +		spin_unlock(&timr->it_lock);
>  	}
>  	rcu_read_unlock();
> +	local_irq_restore(*flags);
> 
>  	return NULL;
>  }
>

[toc] | [prev] | [next] | [standalone]

#1308186

FromThomas Gleixner <tglx@linutronix.de>
Date2016-01-13 10:10 +0100
Message-ID<qQpBf-5Wp-9@gated-at.bofh.it>
In reply to#1307793
Sasha,

On Tue, 12 Jan 2016, Sasha Levin wrote:

Cc'ing Paul, Peter

> While fuzzing with trinity inside a KVM tools guest, running the latest -next
> kernel, I've hit the following lockdep warning:

> [ 3408.474461]  Possible interrupt unsafe locking scenario:
> 
> [ 3408.474461]
> 
> [ 3408.475239]        CPU0                    CPU1
> 
> [ 3408.475809]        ----                    ----
> 
> [ 3408.476380]   lock(&lock->wait_lock);
> 
> [ 3408.476925]                                local_irq_disable();
> 
> [ 3408.477640]                                lock(&(&new_timer->it_lock)->rlock);
>
> [ 3408.478607]                                lock(&lock->wait_lock);

That comes from rcu_read_unlock:

    						rcu_read_unlock()
						 rcu_read_unlock_special()
						 ...
						  rt_mutex_unlock(&rnp->boost_mtx);
   					           raw_spin_lock(&boost_mtx->wait_lock);

> [ 3408.479445]   <Interrupt>
> 
> [ 3408.479796]     lock(&(&new_timer->it_lock)->rlock);

So the task on CPU0 holds rnp->boost_mtx.wait_lock and then the interrupt
deadlocks on the timer->it_lock.

We can fix that particular issue in the posix-timer code by making the
locking symetric:

	rcu_read_lock();
	spin_lock_irq(timer->lock);

...

	spin_unlock_irq(timer->lock);
	rcu_read_unlock();

instead of:

	rcu_read_lock();
	spin_lock_irq(timer->lock);
	rcu_read_unlock();

...

	spin_unlock_irq(timer->lock);

But the question is, whether this is the only offending code path in tree. We
can avoid the hassle by making rtmutex->wait_lock irq safe.

Thoughts?

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1308576

From"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Date2016-01-13 17:30 +0100
Message-ID<qQwt7-2jv-77@gated-at.bofh.it>
In reply to#1308186
On Wed, Jan 13, 2016 at 10:05:49AM +0100, Thomas Gleixner wrote:
> Sasha,
> 
> On Tue, 12 Jan 2016, Sasha Levin wrote:
> 
> Cc'ing Paul, Peter
> 
> > While fuzzing with trinity inside a KVM tools guest, running the latest -next
> > kernel, I've hit the following lockdep warning:
> 
> > [ 3408.474461]  Possible interrupt unsafe locking scenario:
> > 
> > [ 3408.474461]
> > 
> > [ 3408.475239]        CPU0                    CPU1
> > 
> > [ 3408.475809]        ----                    ----
> > 
> > [ 3408.476380]   lock(&lock->wait_lock);
> > 
> > [ 3408.476925]                                local_irq_disable();
> > 
> > [ 3408.477640]                                lock(&(&new_timer->it_lock)->rlock);
> >
> > [ 3408.478607]                                lock(&lock->wait_lock);
> 
> That comes from rcu_read_unlock:
> 
>     						rcu_read_unlock()
> 						 rcu_read_unlock_special()
> 						 ...
> 						  rt_mutex_unlock(&rnp->boost_mtx);
>    					           raw_spin_lock(&boost_mtx->wait_lock);
> 
> > [ 3408.479445]   <Interrupt>
> > 
> > [ 3408.479796]     lock(&(&new_timer->it_lock)->rlock);
> 
> So the task on CPU0 holds rnp->boost_mtx.wait_lock and then the interrupt
> deadlocks on the timer->it_lock.
> 
> We can fix that particular issue in the posix-timer code by making the
> locking symetric:
> 
> 	rcu_read_lock();
> 	spin_lock_irq(timer->lock);
> 
> ...
> 
> 	spin_unlock_irq(timer->lock);
> 	rcu_read_unlock();
> 
> instead of:
> 
> 	rcu_read_lock();
> 	spin_lock_irq(timer->lock);
> 	rcu_read_unlock();
> 
> ...
> 
> 	spin_unlock_irq(timer->lock);
> 
> But the question is, whether this is the only offending code path in tree. We
> can avoid the hassle by making rtmutex->wait_lock irq safe.
> 
> Thoughts?

Given that the lock is disabling irq, I don't see a problem with
extending the RCU read-side critical section to cover the entire
irq-disabled region.  Your point about the hassle of finding and fixing
all the other instances of this sort is well taken, however.

							Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1309542

FromThomas Gleixner <tglx@linutronix.de>
Date2016-01-14 18:50 +0100
Message-ID<qQUc3-1S2-27@gated-at.bofh.it>
In reply to#1308576
On Wed, 13 Jan 2016, Paul E. McKenney wrote:
> On Wed, Jan 13, 2016 at 10:05:49AM +0100, Thomas Gleixner wrote:
> > We can fix that particular issue in the posix-timer code by making the
> > locking symetric:
> > 
> > 	rcu_read_lock();
> > 	spin_lock_irq(timer->lock);
> > 
> > ...
> > 
> > 	spin_unlock_irq(timer->lock);
> > 	rcu_read_unlock();
> > 
> > instead of:
> > 
> > 	rcu_read_lock();
> > 	spin_lock_irq(timer->lock);
> > 	rcu_read_unlock();
> > 
> > ...
> > 
> > 	spin_unlock_irq(timer->lock);
> > 
> > But the question is, whether this is the only offending code path in tree. We
> > can avoid the hassle by making rtmutex->wait_lock irq safe.
> > 
> > Thoughts?
> 
> Given that the lock is disabling irq, I don't see a problem with
> extending the RCU read-side critical section to cover the entire
> irq-disabled region.

I cannot follow here. What would be different if the lock would not disable
irqs? I mean you can get preempted right after rcu_read_lock() before
acquiring the spinlock.

> Your point about the hassle of finding and fixing all the other instances of
> this sort is well taken, however.

Right. We have the pattern 

     rcu_read_lock();
     x = lookup();
     if (x)
	   keep_hold(x)
     rcu_read_unlock();
     return x;

all over the place. Now that keep_hold() can be everything from a refcount to
a spinlock and I'm not sure that we can force stuff depending on the mechanism
to be completely symetric. So we are probably better off by making that rcu
unlock machinery more robust.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1309603

From"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Date2016-01-14 20:40 +0100
Message-ID<qQVUu-3bt-13@gated-at.bofh.it>
In reply to#1309542
On Thu, Jan 14, 2016 at 06:43:16PM +0100, Thomas Gleixner wrote:
> On Wed, 13 Jan 2016, Paul E. McKenney wrote:
> > On Wed, Jan 13, 2016 at 10:05:49AM +0100, Thomas Gleixner wrote:
> > > We can fix that particular issue in the posix-timer code by making the
> > > locking symetric:
> > > 
> > > 	rcu_read_lock();
> > > 	spin_lock_irq(timer->lock);
> > > 
> > > ...
> > > 
> > > 	spin_unlock_irq(timer->lock);
> > > 	rcu_read_unlock();
> > > 
> > > instead of:
> > > 
> > > 	rcu_read_lock();
> > > 	spin_lock_irq(timer->lock);
> > > 	rcu_read_unlock();
> > > 
> > > ...
> > > 
> > > 	spin_unlock_irq(timer->lock);
> > > 
> > > But the question is, whether this is the only offending code path in tree. We
> > > can avoid the hassle by making rtmutex->wait_lock irq safe.
> > > 
> > > Thoughts?
> > 
> > Given that the lock is disabling irq, I don't see a problem with
> > extending the RCU read-side critical section to cover the entire
> > irq-disabled region.
> 
> I cannot follow here. What would be different if the lock would not disable
> irqs? I mean you can get preempted right after rcu_read_lock() before
> acquiring the spinlock.

I was thinking in terms of the fact that disabling irqs would block the
grace period for the current implementation of RCU (but -not- SRCU, just
for the record).  You are right that the new version can be preempted
just after the rcu_read_lock() but the same is true of the old pattern
as well.  To avoid this possibility of preemption, the code would need
to look something like this:

	local_irq_disable();
	rcu_read_lock();
	spin_lock(timer->lock);

...

	spin_unlock(timer->lock);
	rcu_read_unlock();
	local_irq_enable();

> > Your point about the hassle of finding and fixing all the other instances of
> > this sort is well taken, however.
> 
> Right. We have the pattern 
> 
>      rcu_read_lock();
>      x = lookup();
>      if (x)
> 	   keep_hold(x)
>      rcu_read_unlock();
>      return x;
> 
> all over the place. Now that keep_hold() can be everything from a refcount to
> a spinlock and I'm not sure that we can force stuff depending on the mechanism
> to be completely symetric. So we are probably better off by making that rcu
> unlock machinery more robust.

OK.  If I read the lockdep reports correctly, the issue occurs
when rcu_read_unlock_special() finds that it needs to unboost,
which means doing an rt_mutex_unlock().  This is done outside of
rcu_read_unlock_special()'s irq-disabled region, but of course the caller
might have disabled irqs.

If I remember correctly, disabling irqs across rt_mutex_unlock() gets
me lockdep splats.

I could imagine having a per-CPU pointer to rt_mutex that
rcu_read_unlock() sets, and that is checked at every point that irqs
are enabled, with a call to rt_mutex_unlock() if that pointer is non-NULL.

But perhaps you had something else in mind?

							Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1309608

FromThomas Gleixner <tglx@linutronix.de>
Date2016-01-14 20:50 +0100
Message-ID<qQW4a-3gr-13@gated-at.bofh.it>
In reply to#1309603
On Thu, 14 Jan 2016, Paul E. McKenney wrote:
> On Thu, Jan 14, 2016 at 06:43:16PM +0100, Thomas Gleixner wrote:
> > Right. We have the pattern 
> > 
> >      rcu_read_lock();
> >      x = lookup();
> >      if (x)
> > 	   keep_hold(x)
> >      rcu_read_unlock();
> >      return x;
> > 
> > all over the place. Now that keep_hold() can be everything from a refcount to
> > a spinlock and I'm not sure that we can force stuff depending on the mechanism
> > to be completely symetric. So we are probably better off by making that rcu
> > unlock machinery more robust.
> 
> OK.  If I read the lockdep reports correctly, the issue occurs
> when rcu_read_unlock_special() finds that it needs to unboost,
> which means doing an rt_mutex_unlock().  This is done outside of
> rcu_read_unlock_special()'s irq-disabled region, but of course the caller
> might have disabled irqs.
> 
> If I remember correctly, disabling irqs across rt_mutex_unlock() gets
> me lockdep splats.

That shouldn't be the case. The splats come from this scenario:

CPU0 	       	      	    CPU1
rtmutex_lock(rcu)		    
  raw_spin_lock(&rcu->lock)
			    rcu_read_lock()
Interrupt		    spin_lock_irq(some->lock);
			    rcu_read_unlock()
			      rcu_read_unlock_special()
			        rtmutex_unlock(rcu)
				  raw_spin_lock(&rcu->lock)
  spin_lock(some->lock)

Now we dead locked.
 
> I could imagine having a per-CPU pointer to rt_mutex that
> rcu_read_unlock() sets, and that is checked at every point that irqs
> are enabled, with a call to rt_mutex_unlock() if that pointer is non-NULL.
> 
> But perhaps you had something else in mind?

We can solve that issue by taking rtmutex->wait_lock with irqsave. So the
above becomes:

CPU0				CPU1
rtmutex_lock(rcu)		    
  raw_spin_lock_irq(&rcu->lock)
				rcu_read_lock()
		    	    	spin_lock_irq(some->lock);
			    	rcu_read_unlock()
				  rcu_read_unlock_special()
			          rtmutex_unlock(rcu)
				    raw_spin_lock_irqsave(&rcu->lock, flags)
  raw_spin_unlock_irq(&rcu->lock)
Interrupt			    ...
  spin_lock(some->lock)		    raw_spin_unlock_irqrestore(&rcu->lock, flags)
				...
		    	    	spin_unlock_irq(some->lock);
				
Untested patch below.

Thanks,

	tglx

8<-------------------------

Subject: rtmutex: Make wait_lock irq safe
From: Thomas Gleixner <tglx@linutronix.de>
Date: Wed, 13 Jan 2016 11:25:38 +0100

Add some blurb here.

Not-Yet-Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/futex.c           |   18 +++----
 kernel/locking/rtmutex.c |  111 +++++++++++++++++++++++++----------------------
 2 files changed, 69 insertions(+), 60 deletions(-)

--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -1226,7 +1226,7 @@ static int wake_futex_pi(u32 __user *uad
 	if (pi_state->owner != current)
 		return -EINVAL;
 
-	raw_spin_lock(&pi_state->pi_mutex.wait_lock);
+	raw_spin_lock_irq(&pi_state->pi_mutex.wait_lock);
 	new_owner = rt_mutex_next_owner(&pi_state->pi_mutex);
 
 	/*
@@ -1252,22 +1252,22 @@ static int wake_futex_pi(u32 __user *uad
 	else if (curval != uval)
 		ret = -EINVAL;
 	if (ret) {
-		raw_spin_unlock(&pi_state->pi_mutex.wait_lock);
+		raw_spin_unlock_irq(&pi_state->pi_mutex.wait_lock);
 		return ret;
 	}
 
-	raw_spin_lock_irq(&pi_state->owner->pi_lock);
+	raw_spin_lock(&pi_state->owner->pi_lock);
 	WARN_ON(list_empty(&pi_state->list));
 	list_del_init(&pi_state->list);
-	raw_spin_unlock_irq(&pi_state->owner->pi_lock);
+	raw_spin_unlock(&pi_state->owner->pi_lock);
 
-	raw_spin_lock_irq(&new_owner->pi_lock);
+	raw_spin_lock(&new_owner->pi_lock);
 	WARN_ON(!list_empty(&pi_state->list));
 	list_add(&pi_state->list, &new_owner->pi_state_list);
 	pi_state->owner = new_owner;
-	raw_spin_unlock_irq(&new_owner->pi_lock);
+	raw_spin_unlock(&new_owner->pi_lock);
 
-	raw_spin_unlock(&pi_state->pi_mutex.wait_lock);
+	raw_spin_unlock_irq(&pi_state->pi_mutex.wait_lock);
 
 	deboost = rt_mutex_futex_unlock(&pi_state->pi_mutex, &wake_q);
 
@@ -2162,11 +2162,11 @@ static int fixup_owner(u32 __user *uaddr
 		 * we returned due to timeout or signal without taking the
 		 * rt_mutex. Too late.
 		 */
-		raw_spin_lock(&q->pi_state->pi_mutex.wait_lock);
+		raw_spin_lock_irq(&q->pi_state->pi_mutex.wait_lock);
 		owner = rt_mutex_owner(&q->pi_state->pi_mutex);
 		if (!owner)
 			owner = rt_mutex_next_owner(&q->pi_state->pi_mutex);
-		raw_spin_unlock(&q->pi_state->pi_mutex.wait_lock);
+		raw_spin_unlock_irq(&q->pi_state->pi_mutex.wait_lock);
 		ret = fixup_pi_state_owner(uaddr, q, owner);
 		goto out;
 	}
--- a/kernel/locking/rtmutex.c
+++ b/kernel/locking/rtmutex.c
@@ -99,13 +99,14 @@ static inline void mark_rt_mutex_waiters
  * 2) Drop lock->wait_lock
  * 3) Try to unlock the lock with cmpxchg
  */
-static inline bool unlock_rt_mutex_safe(struct rt_mutex *lock)
+static inline bool unlock_rt_mutex_safe(struct rt_mutex *lock,
+					unsigned long flags)
 	__releases(lock->wait_lock)
 {
 	struct task_struct *owner = rt_mutex_owner(lock);
 
 	clear_rt_mutex_waiters(lock);
-	raw_spin_unlock(&lock->wait_lock);
+	raw_spin_unlock_irqrestore(&lock->wait_lock, flags);
 	/*
 	 * If a new waiter comes in between the unlock and the cmpxchg
 	 * we have two situations:
@@ -147,11 +148,12 @@ static inline void mark_rt_mutex_waiters
 /*
  * Simple slow path only version: lock->owner is protected by lock->wait_lock.
  */
-static inline bool unlock_rt_mutex_safe(struct rt_mutex *lock)
+static inline bool unlock_rt_mutex_safe(struct rt_mutex *lock,
+					unsigned long flags)
 	__releases(lock->wait_lock)
 {
 	lock->owner = NULL;
-	raw_spin_unlock(&lock->wait_lock);
+	raw_spin_unlock_irqrestore(&lock->wait_lock, flags);
 	return true;
 }
 #endif
@@ -591,7 +593,7 @@ static int rt_mutex_adjust_prio_chain(st
 		/*
 		 * No requeue[7] here. Just release @task [8]
 		 */
-		raw_spin_unlock_irqrestore(&task->pi_lock, flags);
+		raw_spin_unlock(&task->pi_lock);
 		put_task_struct(task);
 
 		/*
@@ -599,14 +601,14 @@ static int rt_mutex_adjust_prio_chain(st
 		 * If there is no owner of the lock, end of chain.
 		 */
 		if (!rt_mutex_owner(lock)) {
-			raw_spin_unlock(&lock->wait_lock);
+			raw_spin_unlock_irqrestore(&lock->wait_lock, flags);
 			return 0;
 		}
 
 		/* [10] Grab the next task, i.e. owner of @lock */
 		task = rt_mutex_owner(lock);
 		get_task_struct(task);
-		raw_spin_lock_irqsave(&task->pi_lock, flags);
+		raw_spin_lock(&task->pi_lock);
 
 		/*
 		 * No requeue [11] here. We just do deadlock detection.
@@ -621,8 +623,8 @@ static int rt_mutex_adjust_prio_chain(st
 		top_waiter = rt_mutex_top_waiter(lock);
 
 		/* [13] Drop locks */
-		raw_spin_unlock_irqrestore(&task->pi_lock, flags);
-		raw_spin_unlock(&lock->wait_lock);
+		raw_spin_unlock(&task->pi_lock);
+		raw_spin_unlock_irqrestore(&lock->wait_lock, flags);
 
 		/* If owner is not blocked, end of chain. */
 		if (!next_lock)
@@ -643,7 +645,7 @@ static int rt_mutex_adjust_prio_chain(st
 	rt_mutex_enqueue(lock, waiter);
 
 	/* [8] Release the task */
-	raw_spin_unlock_irqrestore(&task->pi_lock, flags);
+	raw_spin_unlock(&task->pi_lock, flags);
 	put_task_struct(task);
 
 	/*
@@ -661,14 +663,14 @@ static int rt_mutex_adjust_prio_chain(st
 		 */
 		if (prerequeue_top_waiter != rt_mutex_top_waiter(lock))
 			wake_up_process(rt_mutex_top_waiter(lock)->task);
-		raw_spin_unlock(&lock->wait_lock);
+		raw_spin_unlock_irqrestore(&lock->wait_lock, flags);
 		return 0;
 	}
 
 	/* [10] Grab the next task, i.e. the owner of @lock */
 	task = rt_mutex_owner(lock);
 	get_task_struct(task);
-	raw_spin_lock_irqsave(&task->pi_lock, flags);
+	raw_spin_lock(&task->pi_lock);
 
 	/* [11] requeue the pi waiters if necessary */
 	if (waiter == rt_mutex_top_waiter(lock)) {
@@ -722,8 +724,8 @@ static int rt_mutex_adjust_prio_chain(st
 	top_waiter = rt_mutex_top_waiter(lock);
 
 	/* [13] Drop the locks */
-	raw_spin_unlock_irqrestore(&task->pi_lock, flags);
-	raw_spin_unlock(&lock->wait_lock);
+	raw_spin_unlock(&task->pi_lock);
+	raw_spin_unlock_irqrestore(&lock->wait_lock, flags);
 
 	/*
 	 * Make the actual exit decisions [12], based on the stored
@@ -766,8 +768,6 @@ static int rt_mutex_adjust_prio_chain(st
 static int try_to_take_rt_mutex(struct rt_mutex *lock, struct task_struct *task,
 				struct rt_mutex_waiter *waiter)
 {
-	unsigned long flags;
-
 	/*
 	 * Before testing whether we can acquire @lock, we set the
 	 * RT_MUTEX_HAS_WAITERS bit in @lock->owner. This forces all
@@ -852,7 +852,7 @@ static int try_to_take_rt_mutex(struct r
 	 * case, but conditionals are more expensive than a redundant
 	 * store.
 	 */
-	raw_spin_lock_irqsave(&task->pi_lock, flags);
+	raw_spin_lock(&task->pi_lock);
 	task->pi_blocked_on = NULL;
 	/*
 	 * Finish the lock acquisition. @task is the new owner. If
@@ -861,7 +861,7 @@ static int try_to_take_rt_mutex(struct r
 	 */
 	if (rt_mutex_has_waiters(lock))
 		rt_mutex_enqueue_pi(task, rt_mutex_top_waiter(lock));
-	raw_spin_unlock_irqrestore(&task->pi_lock, flags);
+	raw_spin_unlock(&task->pi_lock);
 
 takeit:
 	/* We got the lock. */
@@ -894,7 +894,6 @@ static int task_blocks_on_rt_mutex(struc
 	struct rt_mutex_waiter *top_waiter = waiter;
 	struct rt_mutex *next_lock;
 	int chain_walk = 0, res;
-	unsigned long flags;
 
 	/*
 	 * Early deadlock detection. We really don't want the task to
@@ -908,7 +907,7 @@ static int task_blocks_on_rt_mutex(struc
 	if (owner == task)
 		return -EDEADLK;
 
-	raw_spin_lock_irqsave(&task->pi_lock, flags);
+	raw_spin_lock(&task->pi_lock);
 	__rt_mutex_adjust_prio(task);
 	waiter->task = task;
 	waiter->lock = lock;
@@ -921,12 +920,12 @@ static int task_blocks_on_rt_mutex(struc
 
 	task->pi_blocked_on = waiter;
 
-	raw_spin_unlock_irqrestore(&task->pi_lock, flags);
+	raw_spin_unlock(&task->pi_lock);
 
 	if (!owner)
 		return 0;
 
-	raw_spin_lock_irqsave(&owner->pi_lock, flags);
+	raw_spin_lock(&owner->pi_lock);
 	if (waiter == rt_mutex_top_waiter(lock)) {
 		rt_mutex_dequeue_pi(owner, top_waiter);
 		rt_mutex_enqueue_pi(owner, waiter);
@@ -941,7 +940,7 @@ static int task_blocks_on_rt_mutex(struc
 	/* Store the lock on which owner is blocked or NULL */
 	next_lock = task_blocked_on_lock(owner);
 
-	raw_spin_unlock_irqrestore(&owner->pi_lock, flags);
+	raw_spin_unlock(&owner->pi_lock);
 	/*
 	 * Even if full deadlock detection is on, if the owner is not
 	 * blocked itself, we can avoid finding this out in the chain
@@ -957,12 +956,12 @@ static int task_blocks_on_rt_mutex(struc
 	 */
 	get_task_struct(owner);
 
-	raw_spin_unlock(&lock->wait_lock);
+	raw_spin_unlock_irq(&lock->wait_lock);
 
 	res = rt_mutex_adjust_prio_chain(owner, chwalk, lock,
 					 next_lock, waiter, task);
 
-	raw_spin_lock(&lock->wait_lock);
+	raw_spin_lock_irq(&lock->wait_lock);
 
 	return res;
 }
@@ -977,9 +976,8 @@ static void mark_wakeup_next_waiter(stru
 				    struct rt_mutex *lock)
 {
 	struct rt_mutex_waiter *waiter;
-	unsigned long flags;
 
-	raw_spin_lock_irqsave(&current->pi_lock, flags);
+	raw_spin_lock(&current->pi_lock);
 
 	waiter = rt_mutex_top_waiter(lock);
 
@@ -1001,7 +999,7 @@ static void mark_wakeup_next_waiter(stru
 	 */
 	lock->owner = (void *) RT_MUTEX_HAS_WAITERS;
 
-	raw_spin_unlock_irqrestore(&current->pi_lock, flags);
+	raw_spin_unlock(&current->pi_lock);
 
 	wake_q_add(wake_q, waiter->task);
 }
@@ -1018,12 +1016,11 @@ static void remove_waiter(struct rt_mute
 	bool is_top_waiter = (waiter == rt_mutex_top_waiter(lock));
 	struct task_struct *owner = rt_mutex_owner(lock);
 	struct rt_mutex *next_lock;
-	unsigned long flags;
 
-	raw_spin_lock_irqsave(&current->pi_lock, flags);
+	raw_spin_lock(&current->pi_lock);
 	rt_mutex_dequeue(lock, waiter);
 	current->pi_blocked_on = NULL;
-	raw_spin_unlock_irqrestore(&current->pi_lock, flags);
+	raw_spin_unlock(&current->pi_lock);
 
 	/*
 	 * Only update priority if the waiter was the highest priority
@@ -1032,7 +1029,7 @@ static void remove_waiter(struct rt_mute
 	if (!owner || !is_top_waiter)
 		return;
 
-	raw_spin_lock_irqsave(&owner->pi_lock, flags);
+	raw_spin_lock(&owner->pi_lock);
 
 	rt_mutex_dequeue_pi(owner, waiter);
 
@@ -1044,7 +1041,7 @@ static void remove_waiter(struct rt_mute
 	/* Store the lock on which owner is blocked or NULL */
 	next_lock = task_blocked_on_lock(owner);
 
-	raw_spin_unlock_irqrestore(&owner->pi_lock, flags);
+	raw_spin_unlock(&owner->pi_lock);
 
 	/*
 	 * Don't walk the chain, if the owner task is not blocked
@@ -1056,12 +1053,12 @@ static void remove_waiter(struct rt_mute
 	/* gets dropped in rt_mutex_adjust_prio_chain()! */
 	get_task_struct(owner);
 
-	raw_spin_unlock(&lock->wait_lock);
+	raw_spin_unlock_irq(&lock->wait_lock);
 
 	rt_mutex_adjust_prio_chain(owner, RT_MUTEX_MIN_CHAINWALK, lock,
 				   next_lock, NULL, current);
 
-	raw_spin_lock(&lock->wait_lock);
+	raw_spin_lock_irq(&lock->wait_lock);
 }
 
 /*
@@ -1129,13 +1126,13 @@ static int __sched
 				break;
 		}
 
-		raw_spin_unlock(&lock->wait_lock);
+		raw_spin_unlock_irq(&lock->wait_lock);
 
 		debug_rt_mutex_print_deadlock(waiter);
 
 		schedule();
 
-		raw_spin_lock(&lock->wait_lock);
+		raw_spin_lock_irq(&lock->wait_lock);
 		set_current_state(state);
 	}
 
@@ -1172,17 +1169,26 @@ rt_mutex_slowlock(struct rt_mutex *lock,
 		  enum rtmutex_chainwalk chwalk)
 {
 	struct rt_mutex_waiter waiter;
+	unsigned long flags;
 	int ret = 0;
 
 	debug_rt_mutex_init_waiter(&waiter);
 	RB_CLEAR_NODE(&waiter.pi_tree_entry);
 	RB_CLEAR_NODE(&waiter.tree_entry);
 
-	raw_spin_lock(&lock->wait_lock);
+	/*
+	 * Technically we could use raw_spin_[un]lock_irq() here, but this can
+	 * be called in early boot if the cmpxchg() fast path is disabled
+	 * (debug, no architecture support). In this case we will acquire the
+	 * rtmutex with lock->wait_lock held. But we cannot unconditionally
+	 * enable interrupts in that early boot case. So we need to use the
+	 * irqsave/restore variants.
+	 */
+	raw_spin_lock_irqsave(&lock->wait_lock, flags);
 
 	/* Try to acquire the lock again: */
 	if (try_to_take_rt_mutex(lock, current, NULL)) {
-		raw_spin_unlock(&lock->wait_lock);
+		raw_spin_unlock_irqrestore(&lock->wait_lock, flags);
 		return 0;
 	}
 
@@ -1211,7 +1217,7 @@ rt_mutex_slowlock(struct rt_mutex *lock,
 	 */
 	fixup_rt_mutex_waiters(lock);
 
-	raw_spin_unlock(&lock->wait_lock);
+	raw_spin_unlock_irqrestore(&lock->wait_lock, flags);
 
 	/* Remove pending timer: */
 	if (unlikely(timeout))
@@ -1227,6 +1233,7 @@ rt_mutex_slowlock(struct rt_mutex *lock,
  */
 static inline int rt_mutex_slowtrylock(struct rt_mutex *lock)
 {
+	unsigned long flags;
 	int ret;
 
 	/*
@@ -1241,7 +1248,7 @@ static inline int rt_mutex_slowtrylock(s
 	 * The mutex has currently no owner. Lock the wait lock and
 	 * try to acquire the lock.
 	 */
-	raw_spin_lock(&lock->wait_lock);
+	raw_spin_lock_irqsave(&lock->wait_lock, flags);
 
 	ret = try_to_take_rt_mutex(lock, current, NULL);
 
@@ -1251,7 +1258,7 @@ static inline int rt_mutex_slowtrylock(s
 	 */
 	fixup_rt_mutex_waiters(lock);
 
-	raw_spin_unlock(&lock->wait_lock);
+	raw_spin_unlock_irqrestore(&lock->wait_lock, flags);
 
 	return ret;
 }
@@ -1263,7 +1270,9 @@ static inline int rt_mutex_slowtrylock(s
 static bool __sched rt_mutex_slowunlock(struct rt_mutex *lock,
 					struct wake_q_head *wake_q)
 {
-	raw_spin_lock(&lock->wait_lock);
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&lock->wait_lock, flags);
 
 	debug_rt_mutex_unlock(lock);
 
@@ -1302,10 +1311,10 @@ static bool __sched rt_mutex_slowunlock(
 	 */
 	while (!rt_mutex_has_waiters(lock)) {
 		/* Drops lock->wait_lock ! */
-		if (unlock_rt_mutex_safe(lock) == true)
+		if (unlock_rt_mutex_safe(lock, flags) == true)
 			return false;
 		/* Relock the rtmutex and try again */
-		raw_spin_lock(&lock->wait_lock);
+		raw_spin_lock_irqsave(&lock->wait_lock, flags);
 	}
 
 	/*
@@ -1316,7 +1325,7 @@ static bool __sched rt_mutex_slowunlock(
 	 */
 	mark_wakeup_next_waiter(wake_q, lock);
 
-	raw_spin_unlock(&lock->wait_lock);
+	raw_spin_unlock_irqrestore(&lock->wait_lock, flags);
 
 	/* check PI boosting */
 	return true;
@@ -1596,10 +1605,10 @@ int rt_mutex_start_proxy_lock(struct rt_
 {
 	int ret;
 
-	raw_spin_lock(&lock->wait_lock);
+	raw_spin_lock_irq(&lock->wait_lock);
 
 	if (try_to_take_rt_mutex(lock, task, NULL)) {
-		raw_spin_unlock(&lock->wait_lock);
+		raw_spin_unlock_irq(&lock->wait_lock);
 		return 1;
 	}
 
@@ -1620,7 +1629,7 @@ int rt_mutex_start_proxy_lock(struct rt_
 	if (unlikely(ret))
 		remove_waiter(lock, waiter);
 
-	raw_spin_unlock(&lock->wait_lock);
+	raw_spin_unlock_irq(&lock->wait_lock);
 
 	debug_rt_mutex_print_deadlock(waiter);
 
@@ -1668,7 +1677,7 @@ int rt_mutex_finish_proxy_lock(struct rt
 {
 	int ret;
 
-	raw_spin_lock(&lock->wait_lock);
+	raw_spin_lock_irq(&lock->wait_lock);
 
 	set_current_state(TASK_INTERRUPTIBLE);
 
@@ -1684,7 +1693,7 @@ int rt_mutex_finish_proxy_lock(struct rt
 	 */
 	fixup_rt_mutex_waiters(lock);
 
-	raw_spin_unlock(&lock->wait_lock);
+	raw_spin_unlock_irq(&lock->wait_lock);
 
 	return ret;
 }

[toc] | [prev] | [next] | [standalone]

#1309805

From"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Date2016-01-15 02:50 +0100
Message-ID<qR1Gz-7or-17@gated-at.bofh.it>
In reply to#1309608
On Thu, Jan 14, 2016 at 08:47:41PM +0100, Thomas Gleixner wrote:
> On Thu, 14 Jan 2016, Paul E. McKenney wrote:
> > On Thu, Jan 14, 2016 at 06:43:16PM +0100, Thomas Gleixner wrote:
> > > Right. We have the pattern 
> > > 
> > >      rcu_read_lock();
> > >      x = lookup();
> > >      if (x)
> > > 	   keep_hold(x)
> > >      rcu_read_unlock();
> > >      return x;
> > > 
> > > all over the place. Now that keep_hold() can be everything from a refcount to
> > > a spinlock and I'm not sure that we can force stuff depending on the mechanism
> > > to be completely symetric. So we are probably better off by making that rcu
> > > unlock machinery more robust.
> > 
> > OK.  If I read the lockdep reports correctly, the issue occurs
> > when rcu_read_unlock_special() finds that it needs to unboost,
> > which means doing an rt_mutex_unlock().  This is done outside of
> > rcu_read_unlock_special()'s irq-disabled region, but of course the caller
> > might have disabled irqs.
> > 
> > If I remember correctly, disabling irqs across rt_mutex_unlock() gets
> > me lockdep splats.
> 
> That shouldn't be the case. The splats come from this scenario:
> 
> CPU0 	       	      	    CPU1
> rtmutex_lock(rcu)		    
>   raw_spin_lock(&rcu->lock)
> 			    rcu_read_lock()
> Interrupt		    spin_lock_irq(some->lock);
> 			    rcu_read_unlock()
> 			      rcu_read_unlock_special()
> 			        rtmutex_unlock(rcu)
> 				  raw_spin_lock(&rcu->lock)
>   spin_lock(some->lock)
> 
> Now we dead locked.
> 
> > I could imagine having a per-CPU pointer to rt_mutex that
> > rcu_read_unlock() sets, and that is checked at every point that irqs
> > are enabled, with a call to rt_mutex_unlock() if that pointer is non-NULL.
> > 
> > But perhaps you had something else in mind?
> 
> We can solve that issue by taking rtmutex->wait_lock with irqsave. So the
> above becomes:
> 
> CPU0				CPU1
> rtmutex_lock(rcu)		    
>   raw_spin_lock_irq(&rcu->lock)
> 				rcu_read_lock()
> 		    	    	spin_lock_irq(some->lock);
> 			    	rcu_read_unlock()
> 				  rcu_read_unlock_special()
> 			          rtmutex_unlock(rcu)
> 				    raw_spin_lock_irqsave(&rcu->lock, flags)
>   raw_spin_unlock_irq(&rcu->lock)
> Interrupt			    ...
>   spin_lock(some->lock)		    raw_spin_unlock_irqrestore(&rcu->lock, flags)
> 				...
> 		    	    	spin_unlock_irq(some->lock);
> 				
> Untested patch below.

One small fix to make it build below.  Started rcutorture, somewhat
pointlessly given that the splat doesn't appear on my setup.

							Thanx, Paul

------------------------------------------------------------------------

diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
index 18e8b41ff796..1fdf6470dfd1 100644
--- a/kernel/locking/rtmutex.c
+++ b/kernel/locking/rtmutex.c
@@ -645,7 +645,7 @@ static int rt_mutex_adjust_prio_chain(struct task_struct *task,
 	rt_mutex_enqueue(lock, waiter);
 
 	/* [8] Release the task */
-	raw_spin_unlock(&task->pi_lock, flags);
+	raw_spin_unlock(&task->pi_lock);
 	put_task_struct(task);
 
 	/*

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web