From: Will Deacon <will.deacon@arm.com>

The current implementation is not entirely safe in the case that
oprofile_arch_init() fails. We need to make sure that we always call
exit_driverfs() if we've called init_driverfs(). Also, avoid a potential
double free when freeing 'counter_config', e.g. don't free
'counter_config' in both oprofile_arch_init() and oprofile_arch_exit().

Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/oprofile/common.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c
index 0691176..482779c 100644
--- a/arch/arm/oprofile/common.c
+++ b/arch/arm/oprofile/common.c
@@ -275,10 +275,12 @@ out:
 	return ret;
 }
 
-static void  exit_driverfs(void)
+static void exit_driverfs(void)
 {
-	platform_device_unregister(oprofile_pdev);
-	platform_driver_unregister(&oprofile_driver);
+	if (!IS_ERR_OR_NULL(oprofile_pdev)) {
+		platform_device_unregister(oprofile_pdev);
+		platform_driver_unregister(&oprofile_driver);
+	}
 }
 #else
 static int __init init_driverfs(void) { return 0; }
@@ -363,10 +365,8 @@ int __init oprofile_arch_init(struct oprofile_operations *ops)
 	}
 
 	ret = init_driverfs();
-	if (ret) {
-		kfree(counter_config);
+	if (ret)
 		return ret;
-	}
 
 	for_each_possible_cpu(cpu) {
 		perf_events[cpu] = kcalloc(perf_num_counters,
@@ -401,8 +401,9 @@ void oprofile_arch_exit(void)
 	int cpu, id;
 	struct perf_event *event;
 
+	exit_driverfs();
+
 	if (*perf_events) {
-		exit_driverfs();
 		for_each_possible_cpu(cpu) {
 			for (id = 0; id < perf_num_counters; ++id) {
 				event = perf_events[cpu][id];
-- 
1.7.1

--

[ message continues ]

The root cause that makes this check necessary is that
oprofile_arch_exit() is called though oprofile_arch_init() failed. We
should better fix this instead. I have to admit we will then have to

We should not return from oprofile_arch_init() with allocated
resources if the function fails. To fix duplicate kfrees, we should
free it here and then set counter_config to NULL. It should also be
freed if for_each_possible_cpu() or op_name_from_perf_id() fails.

Also, the pointer should be NULLed after freeing in
oprofile_arch_exit(). There, we also don't need the NULL pointer check
as it is save to call kfree(NULL).


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

--

[ message continues ]

Hi Robert,


I took a look through all of the oprofile_arch_{init,exit} functions
and it looks like only ARM needs fixing. Nobody else does any allocation


How about something like this? This removes the exit call
from the init code and sets pointers to NULL after they have been
freed. We still have to do some checking so that we don't try to
release a NULL perf event:

diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c
index 0691176..12253eb 100644
--- a/arch/arm/oprofile/common.c
+++ b/arch/arm/oprofile/common.c
@@ -275,10 +275,12 @@ out:
 	return ret;
 }
 
-static void  exit_driverfs(void)
+static void __exit exit_driverfs(void)
 {
-	platform_device_unregister(oprofile_pdev);
-	platform_driver_unregister(&oprofile_driver);
+	if (!IS_ERR_OR_NULL(oprofile_pdev)) {
+		platform_device_unregister(oprofile_pdev);
+		platform_driver_unregister(&oprofile_driver);
+	}
 }
 #else
 static int __init init_driverfs(void) { return 0; }
@@ -365,6 +367,7 @@ int __init oprofile_arch_init(struct
oprofile_operations *ops)
 	ret = init_driverfs();
 	if (ret) {
 		kfree(counter_config);
+		counter_config = NULL;
 		return ret;
 	}
 
@@ -374,8 +377,10 @@ int __init oprofile_arch_init(struct
oprofile_operations *ops)
 		if (!perf_events[cpu]) {
 			pr_info("oprofile: failed to allocate %d perf events "
 					"for cpu %d\n", perf_num_counters, cpu);
-			while (--cpu >= 0)
+			while (--cpu >= 0) {
 				kfree(perf_events[cpu]);
+				perf_events[cpu] = NULL;
+			}
 			return -ENOMEM;
 		}
 	}
@@ -396,25 +401,27 @@ int __init oprofile_arch_init(struct
oprofile_operations *ops)
 	return ret;
 }
 
-void oprofile_arch_exit(void)
+void __exit oprofile_arch_exit(void)
 {
 	int cpu, id;
 	struct perf_event *event;
 
-	if (*perf_events) {
-		exit_driverfs();
-		for_each_possible_cpu(cpu) {
-			for (id = 0; id < perf_num_counters; ++id) {
-				event = perf_events[cpu][id];
-				if (event != ...
[ message continues ]

I am not sure if it is worth the memory handling code, we could
alternativly implement fixed size arrays with MAX_COUNTERS. This would
eas it a lot. But, this code will become generic, so we can stick with
this implementation.



This check is obsolete here as we do not call oprofile_arch_exit() on



If we shutdown all this in reverse order, this should be after the


	if (event)

A 'return err;' here would remove the goto. I would preffer this.

Thanks,


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

--

[ message continues ]

Hi Robert,

Thanks for taking the time to look at this.

Sorry, I was being braindead and thought oprofile_arch_exit might be
called via oprofile_exit. As it happens, this is only called because of
module_exit so we know that initialisation will have completed.

In light of this and your comments, I've simplified the code. Once
people are happy with it, I'll post it as a couple of patches:

diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c
index 0691176..c2c4a2e 100644
--- a/arch/arm/oprofile/common.c
+++ b/arch/arm/oprofile/common.c
@@ -275,7 +275,7 @@ out:
        return ret;
 }
 
-static void  exit_driverfs(void)
+static void __exit exit_driverfs(void)
 {
        platform_device_unregister(oprofile_pdev);
        platform_driver_unregister(&oprofile_driver);
@@ -359,14 +359,13 @@ int __init oprofile_arch_init(struct oprofile_operations *ops)
        if (!counter_config) {
                pr_info("oprofile: failed to allocate %d "
                                "counters\n", perf_num_counters);
-               return -ENOMEM;
+               ret = -ENOMEM;
+               goto out;
        }
 
        ret = init_driverfs();
-       if (ret) {
-               kfree(counter_config);
-               return ret;
-       }
+       if (ret)
+               goto out;
 
        for_each_possible_cpu(cpu) {
                perf_events[cpu] = kcalloc(perf_num_counters,
@@ -374,9 +373,8 @@ int __init oprofile_arch_init(struct oprofile_operations *ops)
                if (!perf_events[cpu]) {
                        pr_info("oprofile: failed to allocate %d perf events "
                                        "for cpu %d\n", perf_num_counters, cpu);
-                       while (--cpu >= 0)
-                               kfree(perf_events[cpu]);
-                       return -ENOMEM;
+                       ret = -ENOMEM;
+                       goto out;
                }
        }
 
@@ -393,28 +391,33 @@ int __init oprofile_arch_init(struct ...
[ message continues ]

Looks good to me know, the code is a lot easier. Please send me a
final version. I'm going to apply it on Monday.

Thanks Will,


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

--

[ message continues ]