Messages in this thread | | | Date | Fri, 27 Aug 2010 14:43:44 +0200 | From | Robert Richter <> | Subject | Re: [PATCH 1/4] oprofile: Handle initialisation failure more gracefully |
| |
On 26.08.10 15:09:16, Matt Fleming wrote: > 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); > + }
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
| |