linux-kernel - Re: [PATCH] perf_event: fix error handling path

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: <1291915077.6803.1.camel@twins>
Date:	Thu, 09 Dec 2010 18:17:57 +0100
From:	Peter Zijlstra <a.p.zijlstra@...llo.nl>
To:	Corey Ashford <cjashfor@...ux.vnet.ibm.com>
Cc:	jovi zhang <bookjovi@...il.com>,
	Thiago Farina <tfransosi@...il.com>, paulus@...ba.org,
	mingo@...e.hu, acme@...hat.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] perf_event: fix error handling path

On Thu, 2010-12-09 at 09:06 -0800, Corey Ashford wrote:
> On 12/07/2010 05:51 PM, jovi zhang wrote:
> > On Mon, Dec 6, 2010 at 9:59 AM, jovi zhang<bookjovi@...il.com>  wrote:
> >> On Sun, Dec 5, 2010 at 8:29 PM, Thiago Farina<tfransosi@...il.com>  wrote:
> >>>
> >>> On Sat, Dec 4, 2010 at 1:19 AM,<bookjovi@...il.com>  wrote:
> >>>> fix error handling path
> >>>>
> >>>> Signed-off-by: Jovi Zhang<bookjovi@...il.com>
> >>>>   kernel/perf_event.c |    2 --
> >>>>   1 files changed, 0 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/kernel/perf_event.c b/kernel/perf_event.c
> >>>> index cb6c0d2..62f9e9d 100644
> >>>> --- a/kernel/perf_event.c
> >>>> +++ b/kernel/perf_event.c
> >>>> @@ -1918,8 +1918,6 @@ static int get_callchain_buffers(void)
> >>>>         }
> >>>>
> >>>>         err = alloc_callchain_buffers();
> >>>> -       if (err)
> >>>> -               release_callchain_buffers();
> >>>
> >>> Care to explain in the change log message? As I reader, is not clear
> >>> to me what is wrong with this.
> >>
> >> Sorry, the description should be as:
> >> fix error handling path. alloc_callchain_buffers() can return -ENOMEM,
> >> in this time callchain_cpus_entries maybe is NULL, It will oops if
> >> invoke release_callchain_buffers() when callchain_cpus_entries is
> >> NULL.
> >>
> > I hope my understanding is right, is it?
> 
> One possible problem here is what if it returns an error other than 
> -ENOMEM, and the buffers do need to be released?  Maybe you could change 
> the code to
> 
> err = alloc_callchain_buffers();
> if (err != -ENOMEM)
> 	release_callchain_buffers();
> 
> 
> Currently, alloc_callchain_buffers cannot return any error code other 
> than -ENOMEM, but that might change in the future.

The kernel convention is to fully clean up after yourself if you return
an error. So in that sense the patch seems right.

Anyway, anybody care to post a new patch with slightly extended
changelog?
--
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/