From 3b5bac2bdea1de832bdd8e2c904ab7c9479ff9ed Mon Sep 17 00:00:00 2001 From: David Howells Date: Wed, 4 Aug 2010 02:34:17 +0000 Subject: RxRPC: Fix a potential deadlock between the call resend_timer and state_lock RxRPC can potentially deadlock as rxrpc_resend_time_expired() wants to get call->state_lock so that it can alter the state of an RxRPC call. However, its caller (call_timer_fn()) has an apparent lock on the timer struct. The problem is that rxrpc_resend_time_expired() isn't permitted to lock call->state_lock as this could cause a deadlock against rxrpc_send_abort() as that takes state_lock and then attempts to delete the resend timer by calling del_timer_sync(). The deadlock can occur because del_timer_sync() will sit there forever waiting for rxrpc_resend_time_expired() to return, but the latter may then wait for call->state_lock, which rxrpc_send_abort() holds around del_timer_sync()... This leads to a warning appearing in the kernel log that looks something like the attached. It should be sufficient to simply dispense with the locks. It doesn't matter if we set the resend timer expired event bit and queue the event processor whilst we're changing state to one where the resend timer is irrelevant as the event can just be ignored by the processor thereafter. ======================================================= [ INFO: possible circular locking dependency detected ] 2.6.35-rc3-cachefs+ #115 ------------------------------------------------------- swapper/0 is trying to acquire lock: (&call->state_lock){++--..}, at: [] rxrpc_resend_time_expired+0x56/0x96 [af_rxrpc] but task is already holding lock: (&call->resend_timer){+.-...}, at: [] run_timer_softirq+0x182/0x2a5 which lock already depends on the new lock. the existing dependency chain (in reverse order) is: -> #1 (&call->resend_timer){+.-...}: [] __lock_acquire+0x889/0x8fa [] lock_acquire+0x57/0x6d [] del_timer_sync+0x3c/0x86 [] rxrpc_send_abort+0x50/0x97 [af_rxrpc] [] rxrpc_kernel_abort_call+0xa1/0xdd [af_rxrpc] [] afs_deliver_to_call+0x129/0x368 [kafs] [] afs_process_async_call+0x54/0xff [kafs] [] worker_thread+0x1ef/0x2e2 [] kthread+0x7a/0x82 [] kernel_thread_helper+0x4/0x10 -> #0 (&call->state_lock){++--..}: [] validate_chain+0x727/0xd23 [] __lock_acquire+0x889/0x8fa [] lock_acquire+0x57/0x6d [] _raw_read_lock_bh+0x34/0x43 [] rxrpc_resend_time_expired+0x56/0x96 [af_rxrpc] [] run_timer_softirq+0x1f3/0x2a5 [] __do_softirq+0xa2/0x13e [] call_softirq+0x1c/0x28 [] do_softirq+0x38/0x80 [] irq_exit+0x45/0x47 [] smp_apic_timer_interrupt+0x88/0x96 [] apic_timer_interrupt+0x13/0x20 [] cpu_idle+0x4d/0x83 [] start_secondary+0x1bd/0x1c1 other info that might help us debug this: 1 lock held by swapper/0: #0: (&call->resend_timer){+.-...}, at: [] run_timer_softirq+0x182/0x2a5 stack backtrace: Pid: 0, comm: swapper Not tainted 2.6.35-rc3-cachefs+ #115 Call Trace: [] print_circular_bug+0xae/0xbd [] validate_chain+0x727/0xd23 [] __lock_acquire+0x889/0x8fa [] ? mark_lock+0x42f/0x51f [] lock_acquire+0x57/0x6d [] ? rxrpc_resend_time_expired+0x56/0x96 [af_rxrpc] [] _raw_read_lock_bh+0x34/0x43 [] ? rxrpc_resend_time_expired+0x56/0x96 [af_rxrpc] [] rxrpc_resend_time_expired+0x56/0x96 [af_rxrpc] [] run_timer_softirq+0x1f3/0x2a5 [] ? run_timer_softirq+0x182/0x2a5 [] ? rxrpc_resend_time_expired+0x0/0x96 [af_rxrpc] [] ? __do_softirq+0x69/0x13e [] __do_softirq+0xa2/0x13e [] call_softirq+0x1c/0x28 [] do_softirq+0x38/0x80 [] irq_exit+0x45/0x47 [] smp_apic_timer_interrupt+0x88/0x96 [] apic_timer_interrupt+0x13/0x20 [] ? __atomic_notifier_call_chain+0x0/0x86 [] ? mwait_idle+0x6e/0x78 [] ? mwait_idle+0x65/0x78 [] cpu_idle+0x4d/0x83 [] start_secondary+0x1bd/0x1c1 Signed-off-by: David Howells Signed-off-by: David S. Miller --- net/rxrpc/ar-call.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) (limited to 'net/rxrpc/ar-call.c') diff --git a/net/rxrpc/ar-call.c b/net/rxrpc/ar-call.c index 909d092de9f4..bf656c230ba9 100644 --- a/net/rxrpc/ar-call.c +++ b/net/rxrpc/ar-call.c @@ -786,6 +786,7 @@ static void rxrpc_call_life_expired(unsigned long _call) /* * handle resend timer expiry + * - may not take call->state_lock as this can deadlock against del_timer_sync() */ static void rxrpc_resend_time_expired(unsigned long _call) { @@ -796,12 +797,9 @@ static void rxrpc_resend_time_expired(unsigned long _call) if (call->state >= RXRPC_CALL_COMPLETE) return; - read_lock_bh(&call->state_lock); clear_bit(RXRPC_CALL_RUN_RTIMER, &call->flags); - if (call->state < RXRPC_CALL_COMPLETE && - !test_and_set_bit(RXRPC_CALL_RESEND_TIMER, &call->events)) + if (!test_and_set_bit(RXRPC_CALL_RESEND_TIMER, &call->events)) rxrpc_queue_call(call); - read_unlock_bh(&call->state_lock); } /* -- cgit v1.2.2