lkml.org 
[lkml]   [2010]   [Aug]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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



    \
     
     \ /
      Last update: 2010-08-27 14:59    [W:0.027 / U:63.556 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site