linux-kernel - Re: [PATCH v3 10/10] perf: perf interface for uprobes.

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: <20100507022403.GH7426@linux.vnet.ibm.com>
Date:	Fri, 7 May 2010 07:54:03 +0530
From:	Srikar Dronamraju <srikar@...ux.vnet.ibm.com>
To:	Masami Hiramatsu <mhiramat@...hat.com>
Cc:	Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...e.hu>,
	Mel Gorman <mel@....ul.ie>,
	Randy Dunlap <rdunlap@...otime.net>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Roland McGrath <roland@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>,
	Ananth N Mavinakayanahalli <ananth@...ibm.com>,
	Oleg Nesterov <oleg@...hat.com>,
	Mark Wielaard <mjw@...hat.com>,
	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Jim Keniston <jkenisto@...ux.vnet.ibm.com>,
	Frederic Weisbecker <fweisbec@...il.com>,
	"Frank Ch. Eigler" <fche@...hat.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Subject: Re: [PATCH v3 10/10] perf: perf interface for uprobes.

> 
> Could you also rebase your patch on the latest tip tree? :)
> (and remove die() too)

I was aware that I have to rebase the patch to the latest tip.
However I wanted to get a working set out.
Please expect the rebased version in the next version of the posting.

> 
> > Here is a terminal snapshot of placing, using and removing a probe on a
> > process with pid 3329 (corresponding to zsh)
> > 
> > [ Probing a function in the executable using function name  ]
> > -------------------------------------------------------------
> > [root@...D]# perf probe -p 3329 zfree@zsh
> 
> Hmm, I'm not sure why we have to specify both of PID and exec name.
> Is that possible to give a symbol as below? (omit exec name)
> 
> # perf probe -p 3329 zfree

Yes, If the symbol name belongs to th execfile then we can omit the
execfile part. However for symbols outside the execname, the
execfile(library file) is a must(atleast for now).

> 
> > Added new event:
> >   probe:p_3329_zfree                       (on 0x446420)
> 
> Hmm, the event name should be simpler, as like as zfree_3329.
> or, we might better make a new event group for each process, as like as
> 'probe_3329:zfree'
> 

Either zfree_3329 or having a new event group for each process is
possible.

> 
> 
> Hmm, if user gave --list with -p, what happened?
> We need to check a bad combination and warn it.
> 

Currently it would ignore -p option. i.e --list overrides.
We could either warn, or error out. What do you suggest?

> >  
> > +	if (session.pid)
> > +		goto end_dwarf;
> > +
> 
> Again, it must be checked that the combination of -p option and dwarf requirement.
> The latest code has perf_probe_event_need_dwarf(), so you can check it easier.
> 

Okay, 

> [...]
> > +	else
> > +		free(pp->function);
> > +	pp->function = zalloc(sizeof(char *) * 20);
> > +	if (!pp->function)
> > +		die("Failed to allocate memory by zalloc.");
> > +	e_snprintf(pp->function, 20, "0x%llx", vaddr);
> > +}
> 
> Well, after rebasing, you'll need to remove die()s from here and
> make it returns errors. 
> 

Okay. 


> [...]
> > @@ -383,7 +450,11 @@ int synthesize_trace_kprobe_event(struct probe_point *pp)
> >  	pp->probes[0] = buf = zalloc(MAX_CMDLEN);
> >  	if (!buf)
> >  		die("Failed to allocate memory by zalloc.");
> > -	ret = e_snprintf(buf, MAX_CMDLEN, "%s+%d", pp->function, pp->offset);
> > +	if (pp->offset)
> > +		ret = e_snprintf(buf, MAX_CMDLEN, "%s+%d", pp->function,
> > +					pp->offset);
> > +	else
> > +		ret = e_snprintf(buf, MAX_CMDLEN, "%s", pp->function);
> >  	if (ret <= 0)
> >  		goto error;
> >  	len = ret;
> 
> Isn't it a cleanup ?

Can you please elaborate?

> 
> > +void add_trace_uprobe_events(struct probe_point *probes, int nr_probes,
> > +			     bool force_add, pid_t pid)
> > +{
> > +	int fd = open_uprobe_events(O_RDWR, O_APPEND);
> > +	add_trace_probe_events(fd, probes, nr_probes, force_add, pid);
> > +}
> 
> Please close fd in the function which opened it.
> 

Okay.

> 
> >  	memset(&sym, 0, sizeof(sym));
> > -	if (!self->kernel) {
> > +	if (self->kernel || symbol_conf.sort_by_name)
> > +		self->adjust_symbols = 0;
> > +	else {
> >  		self->adjust_symbols = (ehdr.e_type == ET_EXEC ||
> >  				elf_section_by_name(elf, &ehdr, &shdr,
> >  						     ".gnu.prelink_undo",
> >  						     NULL) != NULL);
> > -	} else self->adjust_symbols = 0;
> > +	}
> >  
> >  	elf_symtab__for_each_symbol(syms, nr_syms, idx, sym) {
> >  		struct symbol *f;
> 
> This one changes the symbol.c, so I think you'd better make a
> separate patch for this change.

Okay, I will separate it in the next version.
Thanks Masami for the review.

--
Thanks and Regards
Srikar
--
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/