linux-kernel - Re: [PATCH v2 9/9] perf probe: Support SDT markers having reference counter (semaphore)

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: <20180409230830.48118d3c32f7ec448936ed8a@kernel.org>
Date:   Mon, 9 Apr 2018 23:08:30 +0900
From:   Masami Hiramatsu <mhiramat@...nel.org>
To:     Ravi Bangoria <ravi.bangoria@...ux.vnet.ibm.com>
Cc:     oleg@...hat.com, peterz@...radead.org, srikar@...ux.vnet.ibm.com,
        rostedt@...dmis.org, acme@...nel.org, ananth@...ux.vnet.ibm.com,
        akpm@...ux-foundation.org, alexander.shishkin@...ux.intel.com,
        alexis.berlemont@...il.com, corbet@....net,
        dan.j.williams@...el.com, jolsa@...hat.com, kan.liang@...el.com,
        kjlx@...pleofstupid.com, kstewart@...uxfoundation.org,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org, milian.wolff@...b.com, mingo@...hat.com,
        namhyung@...nel.org, naveen.n.rao@...ux.vnet.ibm.com,
        pc@...ibm.com, tglx@...utronix.de, yao.jin@...ux.intel.com,
        fengguang.wu@...el.com, jglisse@...hat.com
Subject: Re: [PATCH v2 9/9] perf probe: Support SDT markers having reference
 counter (semaphore)

On Mon, 9 Apr 2018 13:59:16 +0530
Ravi Bangoria <ravi.bangoria@...ux.vnet.ibm.com> wrote:

> Hi Masami,
> 
> On 04/09/2018 12:58 PM, Masami Hiramatsu wrote:
> > Hi Ravi,
> >
> > On Wed,  4 Apr 2018 14:01:10 +0530
> > Ravi Bangoria <ravi.bangoria@...ux.vnet.ibm.com> wrote:
> >
> >> @@ -2054,15 +2060,21 @@ char *synthesize_probe_trace_command(struct probe_trace_event *tev)
> >>  	}
> >>  
> >>  	/* Use the tp->address for uprobes */
> >> -	if (tev->uprobes)
> >> +	if (tev->uprobes) {
> >>  		err = strbuf_addf(&buf, "%s:0x%lx", tp->module, tp->address);
> >> -	else if (!strncmp(tp->symbol, "0x", 2))
> >> +		if (uprobe_ref_ctr_is_supported() &&
> >> +		    tp->ref_ctr_offset &&
> >> +		    err >= 0)
> >> +			err = strbuf_addf(&buf, "(0x%lx)", tp->ref_ctr_offset);
> > If the kernel doesn't support uprobe_ref_ctr but the event requires
> > to increment uprobe_ref_ctr, I think we should (at least) warn user here.
> 
> pr_debug("A semaphore is associated with %s:%s and seems your kernel doesn't support it.\n"
>          tev->group, tev->event);
> 
> Looks good?

I think it should be pr_warning() and return NULL, since user may not be able to
trace the event even if it is enabled.

> 
> >> @@ -776,14 +784,21 @@ static char *synthesize_sdt_probe_command(struct sdt_note *note,
> >>  {
> >>  	struct strbuf buf;
> >>  	char *ret = NULL, **args;
> >> -	int i, args_count;
> >> +	int i, args_count, err;
> >> +	unsigned long long ref_ctr_offset;
> >>  
> >>  	if (strbuf_init(&buf, 32) < 0)
> >>  		return NULL;
> >>  
> >> -	if (strbuf_addf(&buf, "p:%s/%s %s:0x%llx",
> >> -				sdtgrp, note->name, pathname,
> >> -				sdt_note__get_addr(note)) < 0)
> >> +	err = strbuf_addf(&buf, "p:%s/%s %s:0x%llx",
> >> +			sdtgrp, note->name, pathname,
> >> +			sdt_note__get_addr(note));
> >> +
> >> +	ref_ctr_offset = sdt_note__get_ref_ctr_offset(note);
> >> +	if (uprobe_ref_ctr_is_supported() && ref_ctr_offset && err >= 0)
> >> +		err = strbuf_addf(&buf, "(0x%llx)", ref_ctr_offset);
> > We don't have to care about uprobe_ref_ctr support here, because
> > this information will be just cached, not directly written to
> > uprobe_events.
> 
> Sure, will remove the check.

Thanks!

> 
> Thanks for the review :).
> Ravi
> 


-- 
Masami Hiramatsu <mhiramat@...nel.org>