Re: [PATCH v2] perf: report/annotate: fix segfault problem.

[Arnaldo Carvalho de Melo <acme@redhat.com>]

Em Fri, Apr 03, 2015 at 12:57:21PM +0200, Jiri Olsa escreveu:
> On Fri, Apr 03, 2015 at 05:56:25AM +0000, Wang Nan wrote:
> > perf report and perf annotate are easy to trigger segfault if trace data
> > contain kernel module information like this:

> >  # perf report -D -i ./perf.data

> >  0 0 0x188 [0x50]: PERF_RECORD_MMAP -1/0: [0xffffffbff1018000(0xf068000) @ 0]: x [test_module]

> oops, I was wondering how you'd get such a MMAP record, because we lookup
> modules real paths and store it.. I haven't realized that if we dont find
> it we keep the '[mod]' name :-\
 
> SNIP
 
> > Patch v1 doesn't consider module named as [aaa.bbb].

> >  tools/perf/util/dso.c | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> > @@ -214,12 +214,23 @@ int __kmod_path__parse(struct kmod_path *m, const char *path,
> >  {
> >  	const char *name = strrchr(path, '/');
> >  	const char *ext  = strrchr(path, '.');
> > +	bool is_simple_name = false;
> >  
> >  	memset(m, 0x0, sizeof(*m));
> >  	name = name ? name + 1 : path;
> >  
> > +	/*
> > +	 * '.' is also a valid character. For example: [aaa.bbb] is a
> > +	 * valid module name. '[' should have higher priority than
> > +	 * '.ko' suffix.
> > +	 */
> 
> 
> > +	if ((name[0] == '[') &&	(strncmp(name, "[vdso]", 6) != 0)) {
> 
> maybe also check for '[vsyscall]' ?

I wonder if there are more? Possibly we should look at some other field
to figure it out if this is kernel code, perhaps use this that is set
when synthesizing a module:

        if (machine__is_host(machine))
                event->header.misc = PERF_RECORD_MISC_KERNEL;
        else
                event->header.misc = PERF_RECORD_MISC_GUEST_KERNEL;

?
 
> > +		m->kmod = true;
> > +		is_simple_name = true;
> > +	}
> > +
> >  	/* No extension, just return name. */
> > -	if (ext == NULL) {
> > +	if ((ext == NULL) || is_simple_name) {
> >  		if (alloc_name) {
> >  			m->name = strdup(name);
> >  			return m->name ? 0 : -ENOMEM;
> 
> otherwise it looks ok to me.. please update also the tests/kmod-path.c ;-)
> 
> thanks,
> jirka