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 for Android: free password hash cracker in your pocket

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

Message-ID: <20100929165027.GP13563@erda.amd.com>
Date:	Wed, 29 Sep 2010 18:50:27 +0200
From:	Robert Richter <robert.richter@....com>
To:	Will Deacon <will.deacon@....com>
CC:	Ingo Molnar <mingo@...e.hu>, LKML <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH] oprofile, arm: proper release resources on failure

On 29.09.10 11:39:44, Will Deacon wrote:

> 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.

Yes, I have cleanup code already for this. But as this is an -rc and
stable fix, I didn't want to change too much here. But I will send a
follow-on patch with the cleanup I made.

> 
> 
> > @@ -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:

Yes, these patches are also in my oprofile/core branch and its changes
conflict, but are scheduled for the next merge window. This fix is for
urgent and for 2.6.36. We will then merge back rc7 or 2.6.36 to
oprofile/core and solve the conflicts. But if you don't have something
else for -rc7/.36 that conflicts, this should be ok.

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

--
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/