[PATCH 0/3] oprofile fixes for 3.0

[PATCH 0/3] oprofile fixes for 3.0

[Robert Richter]

Some fixes for 3.0 for review. Will apply them later and send a pull
request.

Thanks

-Robert


The following changes since commit cbf74cea070fa1f705de4712e25d9e56ae6543c7:

  oprofile, x86: Add comments to IBS LVT offset initialization (2011-05-30 16:36:54 +0200)

Robert Richter (3):
      oprofile: Free potentially owned tasks in case of errors
      oprofile: Fix locking dependency in sync_start()
      oprofile, dcookies: Fix possible circular locking dependency

 drivers/oprofile/buffer_sync.c |   21 +++++++++++----------
 fs/dcookies.c                  |    3 +++
 2 files changed, 14 insertions(+), 10 deletions(-)

[PATCH 1/3] oprofile: Free potentially owned tasks in case of errors

[Robert Richter]

After registering the task free notifier we possibly have tasks in our
dying_tasks list. Free them after unregistering the notifier in case
of an error.

Cc: <stable@kernel.org> # .36+
Signed-off-by: Robert Richter <robert.richter@amd.com>
---
 drivers/oprofile/buffer_sync.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/oprofile/buffer_sync.c b/drivers/oprofile/buffer_sync.c
index a3984f4..04250aa 100644
--- a/drivers/oprofile/buffer_sync.c
+++ b/drivers/oprofile/buffer_sync.c
@@ -141,6 +141,13 @@ static struct notifier_block module_load_nb = {
 	.notifier_call = module_load_notify,
 };
 
+static void free_all_tasks(void)
+{
+	/* make sure we don't leak task structs */
+	process_task_mortuary();
+	process_task_mortuary();
+}
+
 int sync_start(void)
 {
 	int err;
@@ -174,6 +181,7 @@ out3:
 	profile_event_unregister(PROFILE_TASK_EXIT, &task_exit_nb);
 out2:
 	task_handoff_unregister(&task_free_nb);
+	free_all_tasks();
 out1:
 	free_cpumask_var(marked_cpus);
 	goto out;
@@ -192,10 +200,7 @@ void sync_stop(void)
 	mutex_unlock(&buffer_mutex);
 	flush_cpu_work();
 
-	/* make sure we don't leak task structs */
-	process_task_mortuary();
-	process_task_mortuary();
-
+	free_all_tasks();
 	free_cpumask_var(marked_cpus);
 }
 
-- 
1.7.5.rc3

[PATCH 2/3] oprofile: Fix locking dependency in sync_start()

[Robert Richter]

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: [<ffffffffa0000e55>] sync_buffer+0x31/0x3ec [oprofile]

but task is already holding lock:
 ((task_exit_notifier).rwsem){++++..}, at: [<ffffffff81058026>] __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){++++..}:
       [<ffffffff8106557f>] lock_acquire+0xf8/0x11e
       [<ffffffff81463a2b>] down_write+0x44/0x67
       [<ffffffff810581c0>] blocking_notifier_chain_register+0x52/0x8b
       [<ffffffff8105a6ac>] profile_event_register+0x2d/0x2f
       [<ffffffffa00013c1>] sync_start+0x47/0xc6 [oprofile]
       [<ffffffffa00001bb>] oprofile_setup+0x60/0xa5 [oprofile]
       [<ffffffffa00014e3>] event_buffer_open+0x59/0x8c [oprofile]
       [<ffffffff810cd3b9>] __dentry_open+0x1eb/0x308
       [<ffffffff810cd59d>] nameidata_to_filp+0x60/0x67
       [<ffffffff810daad6>] do_last+0x5be/0x6b2
       [<ffffffff810dbc33>] path_openat+0xc7/0x360
       [<ffffffff810dbfc5>] do_filp_open+0x3d/0x8c
       [<ffffffff810ccfd2>] do_sys_open+0x110/0x1a9
       [<ffffffff810cd09e>] sys_open+0x20/0x22
       [<ffffffff8146ad4b>] system_call_fastpath+0x16/0x1b

-> #0 (buffer_mutex){+.+...}:
       [<ffffffff81064dfb>] __lock_acquire+0x1085/0x1711
       [<ffffffff8106557f>] lock_acquire+0xf8/0x11e
       [<ffffffff814634f0>] mutex_lock_nested+0x63/0x309
       [<ffffffffa0000e55>] sync_buffer+0x31/0x3ec [oprofile]
       [<ffffffffa0001226>] task_exit_notify+0x16/0x1a [oprofile]
       [<ffffffff81467b96>] notifier_call_chain+0x37/0x63
       [<ffffffff8105803d>] __blocking_notifier_call_chain+0x50/0x67
       [<ffffffff81058068>] blocking_notifier_call_chain+0x14/0x16
       [<ffffffff8105a718>] profile_task_exit+0x1a/0x1c
       [<ffffffff81039e8f>] do_exit+0x2a/0x6fc
       [<ffffffff8103a5e4>] do_group_exit+0x83/0xae
       [<ffffffff8103a626>] sys_exit_group+0x17/0x1b
       [<ffffffff8146ad4b>] 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: [<ffffffff81058026>] __blocking_notifier_call_chain+0x39/0x67

stack backtrace:
Pid: 4447, comm: oprofiled Not tainted 2.6.39-00007-gcf4d8d4 #10
Call Trace:
 [<ffffffff81063193>] print_circular_bug+0xae/0xbc
 [<ffffffff81064dfb>] __lock_acquire+0x1085/0x1711
 [<ffffffffa0000e55>] ? sync_buffer+0x31/0x3ec [oprofile]
 [<ffffffff8106557f>] lock_acquire+0xf8/0x11e
 [<ffffffffa0000e55>] ? sync_buffer+0x31/0x3ec [oprofile]
 [<ffffffff81062627>] ? mark_lock+0x42f/0x552
 [<ffffffffa0000e55>] ? sync_buffer+0x31/0x3ec [oprofile]
 [<ffffffff814634f0>] mutex_lock_nested+0x63/0x309
 [<ffffffffa0000e55>] ? sync_buffer+0x31/0x3ec [oprofile]
 [<ffffffffa0000e55>] sync_buffer+0x31/0x3ec [oprofile]
 [<ffffffff81058026>] ? __blocking_notifier_call_chain+0x39/0x67
 [<ffffffff81058026>] ? __blocking_notifier_call_chain+0x39/0x67
 [<ffffffffa0001226>] task_exit_notify+0x16/0x1a [oprofile]
 [<ffffffff81467b96>] notifier_call_chain+0x37/0x63
 [<ffffffff8105803d>] __blocking_notifier_call_chain+0x50/0x67
 [<ffffffff81058068>] blocking_notifier_call_chain+0x14/0x16
 [<ffffffff8105a718>] profile_task_exit+0x1a/0x1c
 [<ffffffff81039e8f>] do_exit+0x2a/0x6fc
 [<ffffffff81465031>] ? retint_swapgs+0xe/0x13
 [<ffffffff8103a5e4>] do_group_exit+0x83/0xae
 [<ffffffff8103a626>] sys_exit_group+0x17/0x1b
 [<ffffffff8146ad4b>] system_call_fastpath+0x16/0x1b

Reported-by: Marcin Slusarz <marcin.slusarz@gmail.com>
Cc: Carl Love <carll@us.ibm.com>
Cc: <stable@kernel.org> # .36+
Signed-off-by: Robert Richter <robert.richter@amd.com>
---
 drivers/oprofile/buffer_sync.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/oprofile/buffer_sync.c b/drivers/oprofile/buffer_sync.c
index 04250aa..f34b5b2 100644
--- a/drivers/oprofile/buffer_sync.c
+++ b/drivers/oprofile/buffer_sync.c
@@ -155,8 +155,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;
@@ -173,7 +171,6 @@ int sync_start(void)
 	start_cpu_work();
 
 out:
-	mutex_unlock(&buffer_mutex);
 	return err;
 out4:
 	profile_event_unregister(PROFILE_MUNMAP, &munmap_nb);
@@ -190,14 +187,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_cpu_work();
 
 	free_all_tasks();
-- 
1.7.5.rc3

[PATCH 3/3] oprofile, dcookies: Fix possible circular locking

[Robert Richter]

The lockdep warning below detects a possible A->B/B->A locking
dependency of mm->mmap_sem and dcookie_mutex. The order in
sync_buffer() is mm->mmap_sem/dcookie_mutex, while in
sys_lookup_dcookie() it is vice versa.

Fixing it in sys_lookup_dcookie() by unlocking dcookie_mutex before
copy_to_user().

oprofiled/4432 is trying to acquire lock:
 (&mm->mmap_sem){++++++}, at: [<ffffffff810b444b>] might_fault+0x53/0xa3

but task is already holding lock:
 (dcookie_mutex){+.+.+.}, at: [<ffffffff81124d28>] sys_lookup_dcookie+0x45/0x149

which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:

-> #1 (dcookie_mutex){+.+.+.}:
       [<ffffffff8106557f>] lock_acquire+0xf8/0x11e
       [<ffffffff814634f0>] mutex_lock_nested+0x63/0x309
       [<ffffffff81124e5c>] get_dcookie+0x30/0x144
       [<ffffffffa0000fba>] sync_buffer+0x196/0x3ec [oprofile]
       [<ffffffffa0001226>] task_exit_notify+0x16/0x1a [oprofile]
       [<ffffffff81467b96>] notifier_call_chain+0x37/0x63
       [<ffffffff8105803d>] __blocking_notifier_call_chain+0x50/0x67
       [<ffffffff81058068>] blocking_notifier_call_chain+0x14/0x16
       [<ffffffff8105a718>] profile_task_exit+0x1a/0x1c
       [<ffffffff81039e8f>] do_exit+0x2a/0x6fc
       [<ffffffff8103a5e4>] do_group_exit+0x83/0xae
       [<ffffffff8103a626>] sys_exit_group+0x17/0x1b
       [<ffffffff8146ad4b>] system_call_fastpath+0x16/0x1b

-> #0 (&mm->mmap_sem){++++++}:
       [<ffffffff81064dfb>] __lock_acquire+0x1085/0x1711
       [<ffffffff8106557f>] lock_acquire+0xf8/0x11e
       [<ffffffff810b4478>] might_fault+0x80/0xa3
       [<ffffffff81124de7>] sys_lookup_dcookie+0x104/0x149
       [<ffffffff8146ad4b>] system_call_fastpath+0x16/0x1b

other info that might help us debug this:

1 lock held by oprofiled/4432:
 #0:  (dcookie_mutex){+.+.+.}, at: [<ffffffff81124d28>] sys_lookup_dcookie+0x45/0x149

stack backtrace:
Pid: 4432, comm: oprofiled Not tainted 2.6.39-00008-ge5a450d #9
Call Trace:
 [<ffffffff81063193>] print_circular_bug+0xae/0xbc
 [<ffffffff81064dfb>] __lock_acquire+0x1085/0x1711
 [<ffffffff8102ef13>] ? get_parent_ip+0x11/0x42
 [<ffffffff810b444b>] ? might_fault+0x53/0xa3
 [<ffffffff8106557f>] lock_acquire+0xf8/0x11e
 [<ffffffff810b444b>] ? might_fault+0x53/0xa3
 [<ffffffff810d7d54>] ? path_put+0x22/0x27
 [<ffffffff810b4478>] might_fault+0x80/0xa3
 [<ffffffff810b444b>] ? might_fault+0x53/0xa3
 [<ffffffff81124de7>] sys_lookup_dcookie+0x104/0x149
 [<ffffffff8146ad4b>] system_call_fastpath+0x16/0x1b

References: https://bugzilla.kernel.org/show_bug.cgi?id=13809
Cc: <stable@kernel.org> # .27+
Signed-off-by: Robert Richter <robert.richter@amd.com>
---
 fs/dcookies.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/fs/dcookies.c b/fs/dcookies.c
index a21cabd..dda0dc7 100644
--- a/fs/dcookies.c
+++ b/fs/dcookies.c
@@ -178,6 +178,8 @@ SYSCALL_DEFINE(lookup_dcookie)(u64 cookie64, char __user * buf, size_t len)
 	/* FIXME: (deleted) ? */
 	path = d_path(&dcs->path, kbuf, PAGE_SIZE);
 
+	mutex_unlock(&dcookie_mutex);
+
 	if (IS_ERR(path)) {
 		err = PTR_ERR(path);
 		goto out_free;
@@ -194,6 +196,7 @@ SYSCALL_DEFINE(lookup_dcookie)(u64 cookie64, char __user * buf, size_t len)
 
 out_free:
 	kfree(kbuf);
+	return err;
 out:
 	mutex_unlock(&dcookie_mutex);
 	return err;
-- 
1.7.5.rc3

[GIT PULL] oprofile fixes for v3.0

[Robert Richter]

Ingo,

please pull oprofile fixes for v3.0 (tip/perf/urgent):

 git://git.kernel.org/pub/scm/linux/kernel/git/rric/oprofile.git urgent

Thanks,

-Robert


The following changes since commit d7ebe75b065a7c2d58ffc12f9d2e00d5ea4e71eb:

  perf: Fix comments in include/linux/perf_event.h (2011-06-04 12:31:14 +0200)

are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/rric/oprofile.git urgent

Robert Richter (4):
      oprofile, x86: Add comments to IBS LVT offset initialization
      oprofile: Free potentially owned tasks in case of errors
      oprofile: Fix locking dependency in sync_start()
      oprofile, dcookies: Fix possible circular locking dependency

 arch/x86/kernel/apic/apic.c      |    3 ++-
 arch/x86/oprofile/op_model_amd.c |   13 +++++++++----
 drivers/oprofile/buffer_sync.c   |   21 +++++++++++----------
 fs/dcookies.c                    |    3 +++
 4 files changed, 25 insertions(+), 15 deletions(-)

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

Re: [GIT PULL] oprofile fixes for v3.0

[Ingo Molnar]

* Robert Richter <robert.richter@amd.com> wrote:

> Ingo,
> 
> please pull oprofile fixes for v3.0 (tip/perf/urgent):
> 
>  git://git.kernel.org/pub/scm/linux/kernel/git/rric/oprofile.git urgent
> 
> Thanks,
> 
> -Robert
> 
> 
> The following changes since commit d7ebe75b065a7c2d58ffc12f9d2e00d5ea4e71eb:
> 
>   perf: Fix comments in include/linux/perf_event.h (2011-06-04 12:31:14 +0200)
> 
> are available in the git repository at:
>   git://git.kernel.org/pub/scm/linux/kernel/git/rric/oprofile.git urgent
> 
> Robert Richter (4):
>       oprofile, x86: Add comments to IBS LVT offset initialization
>       oprofile: Free potentially owned tasks in case of errors
>       oprofile: Fix locking dependency in sync_start()
>       oprofile, dcookies: Fix possible circular locking dependency
> 
>  arch/x86/kernel/apic/apic.c      |    3 ++-
>  arch/x86/oprofile/op_model_amd.c |   13 +++++++++----
>  drivers/oprofile/buffer_sync.c   |   21 +++++++++++----------
>  fs/dcookies.c                    |    3 +++
>  4 files changed, 25 insertions(+), 15 deletions(-)

Pulled, thanks a lot Robert!

	Ingo

Re: [GIT PULL] oprofile fixes for v3.0

[Robert Richter]

On 08.06.11 10:06:31, Ingo Molnar wrote:
> 
> * Robert Richter <robert.richter@amd.com> wrote:
> 
> > Ingo,
> > 
> > please pull oprofile fixes for v3.0 (tip/perf/urgent):
> > 
> >  git://git.kernel.org/pub/scm/linux/kernel/git/rric/oprofile.git urgent
> > 
> > Thanks,
> > 
> > -Robert
> > 
> > 
> > The following changes since commit d7ebe75b065a7c2d58ffc12f9d2e00d5ea4e71eb:
> > 
> >   perf: Fix comments in include/linux/perf_event.h (2011-06-04 12:31:14 +0200)
> > 
> > are available in the git repository at:
> >   git://git.kernel.org/pub/scm/linux/kernel/git/rric/oprofile.git urgent
> > 
> > Robert Richter (4):
> >       oprofile, x86: Add comments to IBS LVT offset initialization

Ingo,

it seems you pulled (86dd7909c2c4ae3f219a9233bf0f095b05632ecf) from

 cbf74cea070fa1f705de4712e25d9e56ae6543c7 (oprofile, x86: Add comments to IBS LVT offset initialization)

These ones are still missing:

> >       oprofile: Free potentially owned tasks in case of errors
> >       oprofile: Fix locking dependency in sync_start()
> >       oprofile, dcookies: Fix possible circular locking dependency

But oprofile/urgent is

 fe47ae7f53e179d2ef6771024feb000cbb86640f (oprofile, dcookies: Fix possible circular locking dependency)

Is this intended? Maybe a korg git sync issue?

Thanks,

-Robert

> > 
> >  arch/x86/kernel/apic/apic.c      |    3 ++-
> >  arch/x86/oprofile/op_model_amd.c |   13 +++++++++----
> >  drivers/oprofile/buffer_sync.c   |   21 +++++++++++----------
> >  fs/dcookies.c                    |    3 +++
> >  4 files changed, 25 insertions(+), 15 deletions(-)
> 
> Pulled, thanks a lot Robert!
> 
> 	Ingo
> 

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

Re: [GIT PULL] oprofile fixes for v3.0

[Ingo Molnar]

* Robert Richter <robert.richter@amd.com> wrote:

> On 08.06.11 10:06:31, Ingo Molnar wrote:
> > 
> > * Robert Richter <robert.richter@amd.com> wrote:
> > 
> > > Ingo,
> > > 
> > > please pull oprofile fixes for v3.0 (tip/perf/urgent):
> > > 
> > >  git://git.kernel.org/pub/scm/linux/kernel/git/rric/oprofile.git urgent
> > > 
> > > Thanks,
> > > 
> > > -Robert
> > > 
> > > 
> > > The following changes since commit d7ebe75b065a7c2d58ffc12f9d2e00d5ea4e71eb:
> > > 
> > >   perf: Fix comments in include/linux/perf_event.h (2011-06-04 12:31:14 +0200)
> > > 
> > > are available in the git repository at:
> > >   git://git.kernel.org/pub/scm/linux/kernel/git/rric/oprofile.git urgent
> > > 
> > > Robert Richter (4):
> > >       oprofile, x86: Add comments to IBS LVT offset initialization
> 
> Ingo,
> 
> it seems you pulled (86dd7909c2c4ae3f219a9233bf0f095b05632ecf) from
> 
>  cbf74cea070fa1f705de4712e25d9e56ae6543c7 (oprofile, x86: Add comments to IBS LVT offset initialization)
> 
> These ones are still missing:
> 
> > >       oprofile: Free potentially owned tasks in case of errors
> > >       oprofile: Fix locking dependency in sync_start()
> > >       oprofile, dcookies: Fix possible circular locking dependency
> 
> But oprofile/urgent is
> 
>  fe47ae7f53e179d2ef6771024feb000cbb86640f (oprofile, dcookies: Fix possible circular locking dependency)
> 
> Is this intended? Maybe a korg git sync issue?

Yeah, probably - and i reviewed the on-lkml patches you sent earlier 
and didnt notice that these three are missing from the pull request. 
I pulled again and the bits are there now.

Thanks,

	Ingo