From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1030277Ab1LPUyF (ORCPT ); Fri, 16 Dec 2011 15:54:05 -0500 Received: from cantor2.suse.de ([195.135.220.15]:49028 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S964984Ab1LPUwQ (ORCPT ); Fri, 16 Dec 2011 15:52:16 -0500 X-Mailbox-Line: From gregkh@clark.kroah.org Fri Dec 16 12:50:42 2011 Message-Id: <20111216205042.791953117@clark.kroah.org> User-Agent: quilt/0.50-24.1 Date: Fri, 16 Dec 2011 12:49:38 -0800 From: Greg KH To: linux-kernel@vger.kernel.org, stable@vger.kernel.org Cc: torvalds@linux-foundation.org, akpm@linux-foundation.org, alan@lxorguk.ukuu.org.uk, oprofile-list , Carl Love , Robert Richter Subject: [05/16] oprofile: Fix locking dependency in sync_start() In-Reply-To: <20111216205049.GA28030@kroah.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 2.6.32-longterm review patch. If anyone has any objections, please let me know. ------------------ From: Robert Richter commit 130c5ce716c9bfd1c2a2ec840a746eb7ff9ce1e6 upstream. This fixes the A->B/B->A locking dependency, see the warning below. The function task_exit_notify() is called with (task_exit_notifier) .rwsem set and then calls sync_buffer() which locks buffer_mutex. In sync_start() the buffer_mutex was set to prevent notifier functions to be started before sync_start() is finished. But when registering the notifier, (task_exit_notifier).rwsem is locked too, but now in different order than in sync_buffer(). In theory this causes a locking dependency, what does not occur in practice since task_exit_notify() is always called after the notifier is registered which means the lock is already released. However, after checking the notifier functions it turned out the buffer_mutex in sync_start() is unnecessary. This is because sync_buffer() may be called from the notifiers even if sync_start() did not finish yet, the buffers are already allocated but empty. No need to protect this with the mutex. So we fix this theoretical locking dependency by removing buffer_mutex in sync_start(). This is similar to the implementation before commit: 750d857 oprofile: fix crash when accessing freed task structs which introduced the locking dependency. Lockdep warning: oprofiled/4447 is trying to acquire lock: (buffer_mutex){+.+...}, at: [] sync_buffer+0x31/0x3ec [oprofile] but task is already holding lock: ((task_exit_notifier).rwsem){++++..}, at: [] __blocking_notifier_call_chain+0x39/0x67 which lock already depends on the new lock. the existing dependency chain (in reverse order) is: -> #1 ((task_exit_notifier).rwsem){++++..}: [] lock_acquire+0xf8/0x11e [] down_write+0x44/0x67 [] blocking_notifier_chain_register+0x52/0x8b [] profile_event_register+0x2d/0x2f [] sync_start+0x47/0xc6 [oprofile] [] oprofile_setup+0x60/0xa5 [oprofile] [] event_buffer_open+0x59/0x8c [oprofile] [] __dentry_open+0x1eb/0x308 [] nameidata_to_filp+0x60/0x67 [] do_last+0x5be/0x6b2 [] path_openat+0xc7/0x360 [] do_filp_open+0x3d/0x8c [] do_sys_open+0x110/0x1a9 [] sys_open+0x20/0x22 [] system_call_fastpath+0x16/0x1b -> #0 (buffer_mutex){+.+...}: [] __lock_acquire+0x1085/0x1711 [] lock_acquire+0xf8/0x11e [] mutex_lock_nested+0x63/0x309 [] sync_buffer+0x31/0x3ec [oprofile] [] task_exit_notify+0x16/0x1a [oprofile] [] notifier_call_chain+0x37/0x63 [] __blocking_notifier_call_chain+0x50/0x67 [] blocking_notifier_call_chain+0x14/0x16 [] profile_task_exit+0x1a/0x1c [] do_exit+0x2a/0x6fc [] do_group_exit+0x83/0xae [] sys_exit_group+0x17/0x1b [] system_call_fastpath+0x16/0x1b other info that might help us debug this: 1 lock held by oprofiled/4447: #0: ((task_exit_notifier).rwsem){++++..}, at: [] __blocking_notifier_call_chain+0x39/0x67 stack backtrace: Pid: 4447, comm: oprofiled Not tainted 2.6.39-00007-gcf4d8d4 #10 Call Trace: [] print_circular_bug+0xae/0xbc [] __lock_acquire+0x1085/0x1711 [] ? sync_buffer+0x31/0x3ec [oprofile] [] lock_acquire+0xf8/0x11e [] ? sync_buffer+0x31/0x3ec [oprofile] [] ? mark_lock+0x42f/0x552 [] ? sync_buffer+0x31/0x3ec [oprofile] [] mutex_lock_nested+0x63/0x309 [] ? sync_buffer+0x31/0x3ec [oprofile] [] sync_buffer+0x31/0x3ec [oprofile] [] ? __blocking_notifier_call_chain+0x39/0x67 [] ? __blocking_notifier_call_chain+0x39/0x67 [] task_exit_notify+0x16/0x1a [oprofile] [] notifier_call_chain+0x37/0x63 [] __blocking_notifier_call_chain+0x50/0x67 [] blocking_notifier_call_chain+0x14/0x16 [] profile_task_exit+0x1a/0x1c [] do_exit+0x2a/0x6fc [] ? retint_swapgs+0xe/0x13 [] do_group_exit+0x83/0xae [] sys_exit_group+0x17/0x1b [] system_call_fastpath+0x16/0x1b Reported-by: Marcin Slusarz Cc: Carl Love Signed-off-by: Robert Richter Signed-off-by: Greg Kroah-Hartman --- drivers/oprofile/buffer_sync.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) --- a/drivers/oprofile/buffer_sync.c +++ b/drivers/oprofile/buffer_sync.c @@ -154,8 +154,6 @@ int sync_start(void) if (!zalloc_cpumask_var(&marked_cpus, GFP_KERNEL)) return -ENOMEM; - mutex_lock(&buffer_mutex); - err = task_handoff_register(&task_free_nb); if (err) goto out1; @@ -172,7 +170,6 @@ int sync_start(void) start_cpu_work(); out: - mutex_unlock(&buffer_mutex); return err; out4: profile_event_unregister(PROFILE_MUNMAP, &munmap_nb); @@ -189,14 +186,13 @@ out1: void sync_stop(void) { - /* flush buffers */ - mutex_lock(&buffer_mutex); end_cpu_work(); unregister_module_notifier(&module_load_nb); profile_event_unregister(PROFILE_MUNMAP, &munmap_nb); profile_event_unregister(PROFILE_TASK_EXIT, &task_exit_nb); task_handoff_unregister(&task_free_nb); - mutex_unlock(&buffer_mutex); + barrier(); /* do all of the above first */ + flush_scheduled_work(); free_all_tasks();