On 26.08.10 15:09:16, Matt Fleming wrote: > From: Will Deacon <will.deacon@xxxxxxx> > > 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@xxxxxxx> > --- > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-arch" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html