linux-kernel - Re: [PATCH v2 03/14] perf report: create real callchain entries for inlined frames

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]

Date:   Thu, 07 Sep 2017 17:05:34 +0200
From:   Milian Wolff <milian.wolff@...b.com>
To:     Namhyung Kim <namhyung@...nel.org>
Cc:     acme@...nel.org, Jin Yao <yao.jin@...ux.intel.com>,
        Linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org,
        Arnaldo Carvalho de Melo <acme@...hat.com>,
        David Ahern <dsahern@...il.com>,
        Peter Zijlstra <a.p.zijlstra@...llo.nl>, kernel-team@....com
Subject: Re: [PATCH v2 03/14] perf report: create real callchain entries for inlined frames

On Thursday, September 7, 2017 4:58:53 PM CEST Namhyung Kim wrote:
> Hi Milian,

Hey Namhyung!

> > > > > @@ -511,10 +563,63 @@ void inline_node__delete(struct inline_node
> > > > > *node)
> > > > > 
> > > > >  	list_for_each_entry_safe(ilist, tmp, &node->val, list) {
> > > > >  	
> > > > >  		list_del_init(&ilist->list);
> > > > > 
> > > > > -		zfree(&ilist->filename);
> > > > > -		zfree(&ilist->funcname);
> > > > > +		zfree(&ilist->srcline);
> > > > > +		// only the inlined symbols are owned by the list
> > > > > +		if (ilist->symbol && ilist->symbol->inlined)
> > > > > +			symbol__delete(ilist->symbol);
> > > > 
> > > > Existing symbols are released at this moment.
> > > 
> > > Thanks for the review, I'll try to look into these issues once I have
> > > more
> > > time again.
> > 
> > OK, so I just dug into this part of the patch again. I don't think it's
> > actually a problem after all:
> > 
> > When an inline node reuses the real symbol, that symbol won't have its
> > `inlined` member set to true. Thus these symbols will never get deleted by
> > inline_node__delete.
> 
> But ilist->symbol is a dangling pointer so accessing ->inlined would
> be a problem, no?

Sorry, but I can't follow. Why would it be a dangling pointer? Note, again, 
that I've tested this with both valgrind and ASAN and neither reports any 
issues about this code.

> > If you have suggestions on how to make this clearer, I'm
> > all ears. For now, I'll add a comment to where we alias/reuse the symbol.
> > 
> > I'll try to split up the patch now to make it somehow easier to review.
> 
> Thanks for doing this, but I'm afraid I don't have time to review
> before going to OSS-NA.

No problem, enjoy!
-- 
Milian Wolff | milian.wolff@...b.com | Senior Software Engineer
KDAB (Deutschland) GmbH&Co KG, a KDAB Group company
Tel: +49-30-521325470
KDAB - The Qt Experts
Download attachment "smime.p7s" of type "application/pkcs7-signature" (3826 bytes)