linux-kernel - Re: [PATCH] oprofile, arm: proper release resources on failure

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1285774784.30080.19.camel@e102144-lin.cambridge.arm.com>
Date:	Wed, 29 Sep 2010 16:39:44 +0100
From:	Will Deacon <will.deacon@....com>
To:	Robert Richter <robert.richter@....com>
Cc:	Ingo Molnar <mingo@...e.hu>, LKML <linux-kernel@...r.kernel.org>,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] oprofile, arm: proper release resources on failure

On Wed, 2010-09-29 at 15:52 +0100, Robert Richter wrote:
> Will,
> 

Hi Robert,


> the patch below fixes a resource leak I found during code review. Can
> you please review and test it (I don't have an ARM environment for
> this available). If you are fine with the change, please ack. I want
> to send it upstream via tip/perf/urgent.
> 

I have a few minor clarifications:
> 

> diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c
> index 0691176..72e09eb 100644
> --- a/arch/arm/oprofile/common.c
> +++ b/arch/arm/oprofile/common.c
> @@ -102,6 +102,7 @@ static int op_create_counter(int cpu, int event)
>         if (IS_ERR(pevent)) {
>                 ret = PTR_ERR(pevent);
>         } else if (pevent->state != PERF_EVENT_STATE_ACTIVE) {
> +               perf_event_release_kernel(pevent);
>                 pr_warning("oprofile: failed to enable event %d "
>                                 "on CPU %d\n", event, cpu);
>                 ret = -EBUSY;


Yup, this is needed. Thanks! It would be nice to do away with the
else statement altogether but I think adding a pinned event and
then failing to activate it still succeeds in
perf_event_create_kernel_counter.


> @@ -365,6 +366,7 @@ int __init oprofile_arch_init(struct oprofile_operations *ops)
>         ret = init_driverfs();
>         if (ret) {
>                 kfree(counter_config);
> +               counter_config = NULL;
>                 return ret;
>         }
> 
> @@ -402,7 +404,6 @@ void oprofile_arch_exit(void)
>         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];
> @@ -413,8 +414,10 @@ void oprofile_arch_exit(void)
>                 }
>         }
> 
> -       if (counter_config)
> +       if (counter_config) {
>                 kfree(counter_config);
> +               exit_driverfs();
> +       }
>  }
>  #else
>  int __init oprofile_arch_init(struct oprofile_operations *ops)
> --
> 1.7.2.2
> 

Hmm, these three hunks conflict with the patches I posted last
month to fix the resource allocation and freeing. Can't we
merge those patches instead? I have versions against -rc6 here:

git://linux-arm.org/linux-2.6-wd.git oprofile-mm

Cheers,

Will

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/