From mboxrd@z Thu Jan 1 00:00:00 1970 From: Robert Richter Subject: Re: [PATCH 1/4] oprofile: Handle initialisation failure more gracefully Date: Fri, 27 Aug 2010 14:43:44 +0200 Message-ID: <20100827124344.GJ22783@erda.amd.com> References: <442a16796990290ca3ebaaa3d0ab317e7b0a30a5.1282848651.git.matt@console-pimps.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Return-path: Received: from tx2ehsobe005.messaging.microsoft.com ([65.55.88.15]:49066 "EHLO TX2EHSOBE010.bigfish.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752782Ab0H0M4Q (ORCPT ); Fri, 27 Aug 2010 08:56:16 -0400 Content-Disposition: inline In-Reply-To: <442a16796990290ca3ebaaa3d0ab317e7b0a30a5.1282848651.git.matt@console-pimps.org> Sender: linux-arch-owner@vger.kernel.org List-ID: To: Matt Fleming Cc: "linux-kernel@vger.kernel.org" , Will Deacon , Paul Mundt , Russell King , "linux-arm-kernel@lists.infradead.org" , "linux-sh@vger.kernel.org" , Peter Zijlstra , Ingo Molnar , Frederic Weisbecker , Arnaldo Carvalho de Melo , "linux-arch@vger.kernel.org" On 26.08.10 15:09:16, Matt Fleming wrote: > From: Will Deacon > > 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 > --- > 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); > + } 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 check all architectural implementations. > } > #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); 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). -Robert > + 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 > > -- Advanced Micro Devices, Inc. Operating System Research Center From mboxrd@z Thu Jan 1 00:00:00 1970 From: Robert Richter Date: Fri, 27 Aug 2010 12:43:44 +0000 Subject: Re: [PATCH 1/4] oprofile: Handle initialisation failure more Message-Id: <20100827124344.GJ22783@erda.amd.com> List-Id: References: <442a16796990290ca3ebaaa3d0ab317e7b0a30a5.1282848651.git.matt@console-pimps.org> In-Reply-To: <442a16796990290ca3ebaaa3d0ab317e7b0a30a5.1282848651.git.matt@console-pimps.org> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Matt Fleming Cc: "linux-kernel@vger.kernel.org" , Will Deacon , Paul Mundt , Russell King , "linux-arm-kernel@lists.infradead.org" , "linux-sh@vger.kernel.org" , Peter Zijlstra , Ingo Molnar , Frederic Weisbecker , Arnaldo Carvalho de Melo , "linux-arch@vger.kernel.org" On 26.08.10 15:09:16, Matt Fleming wrote: > From: Will Deacon > > 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 > --- > 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); > + } 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 check all architectural implementations. > } > #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); 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). -Robert > + 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 > > -- Advanced Micro Devices, Inc. Operating System Research Center From mboxrd@z Thu Jan 1 00:00:00 1970 From: robert.richter@amd.com (Robert Richter) Date: Fri, 27 Aug 2010 14:43:44 +0200 Subject: [PATCH 1/4] oprofile: Handle initialisation failure more gracefully In-Reply-To: <442a16796990290ca3ebaaa3d0ab317e7b0a30a5.1282848651.git.matt@console-pimps.org> References: <442a16796990290ca3ebaaa3d0ab317e7b0a30a5.1282848651.git.matt@console-pimps.org> Message-ID: <20100827124344.GJ22783@erda.amd.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 26.08.10 15:09:16, Matt Fleming wrote: > From: Will Deacon > > 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 > --- > 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); > + } 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 check all architectural implementations. > } > #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); 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). -Robert > + 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 > > -- Advanced Micro Devices, Inc. Operating System Research Center