| 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
| ||
|
Message-ID: <525403D8.1010204@linux.vnet.ibm.com>
Date: Tue, 08 Oct 2013 18:38:40 +0530
From: Hemant <hkshaw@...ux.vnet.ibm.com>
To: Namhyung Kim <namhyung@...nel.org>
CC: linux-kernel@...r.kernel.org, srikar@...ux.vnet.ibm.com,
peterz@...radead.org, oleg@...hat.com,
hegdevasant@...ux.vnet.ibm.com, mingo@...hat.com, anton@...hat.com,
systemtap@...rceware.org, masami.hiramatsu.pt@...achi.com,
aravinda@...ux.vnet.ibm.com
Subject: Re: [PATCH v2 2/3] Support for perf to probe into SDT markers:
On 10/08/2013 02:39 PM, Namhyung Kim wrote:
> [...]
> +
> + tmp = strdup(ptr);
> + if (!tmp)
> + return -ENOMEM;
> These -ENOMEM returning should free all memory region allocated
> previously.
Yes, missed that.
>
>> + pev->point.note->name = tmp;
>> + pev->group = pev->point.note->provider;
>> + if (!pev->event)
>> + pev->event = pev->point.note->name;
>> + pev->sdt = true;
>> + return 0;
>> + }
>> ptr = strpbrk(arg, ";:+@%");
>> if (ptr) {
>> nc = *ptr;
>> @@ -1270,6 +1299,13 @@ static char *synthesize_perf_probe_point(struct perf_probe_point *pp)
>> ret = e_snprintf(buf, MAX_CMDLEN, "%s%s%s%s%s", pp->function,
>> offs, pp->retprobe ? "%return" : "", line,
>> file);
>> + else if (pp->note)
>> + if (pp->note->bit32)
>> + ret = e_snprintf(buf, MAX_CMDLEN, "0x%x",
>> + pp->note->addr.a32[0]);
>> + else
>> + ret = e_snprintf(buf, MAX_CMDLEN, "0x%lx",
>> + pp->note->addr.a64[0]);
> This kind of code tends to cause 32/64-bit build problem. Did you test
> it on a 32-bit system too? Anyway, I think things like below should
> work:
>
> unsigned long long addr;
>
> if (pp->note->bit32)
> addr = pp->note->addr.a32[0];
> else
> addr = pp->note->addr.a64[0];
>
> ret = e_snprintf(buf, MAX_CMDLEN, "0x%llx", addr);
>
Looks better, thanks, will make the required changes.
>> else
>> ret = e_snprintf(buf, MAX_CMDLEN, "%s%s", file, line);
>> if (ret <= 0)
>> @@ -1923,6 +1959,19 @@ static void cleanup_sdt_note_list(struct list_head *sdt_notes)
>> }
>> }
>>
>> +static int try_to_find_sdt_notes(struct perf_probe_event *pev,
>> + const char *target)
>> +{
>> + struct list_head sdt_notes;
>> + int ret = -1;
>> +
>> + INIT_LIST_HEAD(&sdt_notes);
> You can use just LIST_HEAD(sdt_notes) here.
>
Ok.
>> + ret = search_sdt_note(pev->point.note, &sdt_notes, target);
>> + if (!list_empty(&sdt_notes))
>> + cleanup_sdt_note_list(&sdt_notes);
>> + return ret;
>> +}
>> +
>> static int convert_to_probe_trace_events(struct perf_probe_event *pev,
>> struct probe_trace_event **tevs,
>> int max_tevs, const char *target)
>> @@ -1930,11 +1979,20 @@ static int convert_to_probe_trace_events(struct perf_probe_event *pev,
>> struct symbol *sym;
>> int ret = 0, i;
>> struct probe_trace_event *tev;
>> + char *buf;
>>
>> - /* Convert perf_probe_event with debuginfo */
>> - ret = try_to_find_probe_trace_events(pev, tevs, max_tevs, target);
>> - if (ret != 0)
>> - return ret; /* Found in debuginfo or got an error */
>> + if (pev->sdt) {
>> + ret = try_to_find_sdt_notes(pev, target);
>> + if (ret)
>> + return ret;
>> + } else {
>> + /* Convert perf_probe_event with debuginfo */
>> + ret = try_to_find_probe_trace_events(pev, tevs, max_tevs,
>> + target);
>> + /* Found in debuginfo or got an error */
>> + if (ret != 0)
>> + return ret;
>> + }
>>
>> /* Allocate trace event buffer */
>> tev = *tevs = zalloc(sizeof(struct probe_trace_event));
>> @@ -1942,10 +2000,25 @@ static int convert_to_probe_trace_events(struct perf_probe_event *pev,
>> return -ENOMEM;
>>
>> /* Copy parameters */
>> - tev->point.symbol = strdup(pev->point.function);
>> - if (tev->point.symbol == NULL) {
>> - ret = -ENOMEM;
>> - goto error;
>> + if (pev->sdt) {
>> + buf = (char *)zalloc(sizeof(char) * MAX_CMDLEN);
>> + if (!buf) {
>> + ret = -ENOMEM;
>> + goto error;
>> + }
>> + if (pev->point.note->bit32)
>> + sprintf(buf, "0x%x",
>> + (unsigned)pev->point.note->addr.a32[0]);
>> + else
>> + sprintf(buf, "0x%lx",
>> + (unsigned long)pev->point.note->addr.a64[0]);
> Please see my comment above.
Ok. Will change that too.
>
>> + tev->point.symbol = buf;
>> + } else {
>> + tev->point.symbol = strdup(pev->point.function);
>> + if (tev->point.symbol == NULL) {
>> + ret = -ENOMEM;
>> + goto error;
>> + }
>> }
>>
>> if (target) {
>> diff --git a/tools/perf/util/probe-event.h b/tools/perf/util/probe-event.h
>> index b8a9347..4bd50cc 100644
>> --- a/tools/perf/util/probe-event.h
>> +++ b/tools/perf/util/probe-event.h
>> @@ -47,6 +47,7 @@ struct perf_probe_point {
>> bool retprobe; /* Return probe flag */
>> char *lazy_line; /* Lazy matching pattern */
>> unsigned long offset; /* Offset from function entry */
>> + struct sdt_note *note;
> I guess what you need is a 'struct list_head'.
Yes, struct list_head will be a better choice in struct perf_probe_point.
>
>> };
>>
>> /* Perf probe probing argument field chain */
>> @@ -72,6 +73,7 @@ struct perf_probe_event {
>> struct perf_probe_point point; /* Probe point */
>> int nargs; /* Number of arguments */
>> bool uprobes;
>> + bool sdt;
>> struct perf_probe_arg *args; /* Arguments */
>> };
>>
>> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
>> index 6b17260..d0e7a66 100644
>> --- a/tools/perf/util/symbol-elf.c
>> +++ b/tools/perf/util/symbol-elf.c
>> @@ -1022,6 +1022,86 @@ out_ret:
>> return ret;
>> }
>>
>> +static void adjust_note_addr(struct sdt_note *tmp, struct sdt_note *key,
>> + Elf *elf)
>> +{
>> + GElf_Ehdr ehdr;
>> + GElf_Addr base_off = 0;
>> + GElf_Shdr shdr;
>> +
>> + if (!gelf_getehdr(elf, &ehdr)) {
>> + pr_debug("%s : cannot get elf header.\n", __func__);
>> + return;
>> + }
>> +
>> + /*
>> + * Find out the .stapsdt.base section.
>> + * This scn will help us to handle prelinking (if present).
>> + * Compare the retrieved file offset of the base section with the
>> + * base address in the description of the SDT note. If its different,
>> + * then accordingly, adjust the note location.
>> + */
>> + if (elf_section_by_name(elf, &ehdr, &shdr, SDT_BASE, NULL))
>> + base_off = shdr.sh_offset;
>> + if (base_off) {
>> + if (tmp->bit32)
>> + key->addr.a32[0] = tmp->addr.a32[0] + base_off -
>> + tmp->addr.a32[1];
>> + else
>> + key->addr.a64[0] = tmp->addr.a64[0] + base_off -
>> + tmp->addr.a64[1];
>> + }
>> + key->bit32 = tmp->bit32;
>> +}
>> +
>> +int search_sdt_note(struct sdt_note *key, struct list_head *sdt_notes,
>> + const char *target)
>> +{
>> + Elf *elf;
>> + int fd, ret = -1;
>> + struct list_head *pos = NULL, *head = NULL;
>> + struct sdt_note *tmp = NULL;
>> +
>> + fd = open(target, O_RDONLY);
>> + if (fd < 0) {
>> + pr_err("Failed to open %s\n", target);
>> + goto out_ret;
>> + }
>> +
>> + symbol__elf_init();
>> + elf = elf_begin(fd, PERF_ELF_C_READ_MMAP, NULL);
>> + if (!elf) {
>> + pr_err("%s : can't read %s ELF file.\n", __func__, target);
>> + goto out_close;
>> + }
>> +
>> + head = construct_sdt_notes_list(elf);
>> + if (!head)
>> + goto out_end;
>> +
>> + list_add(sdt_notes, head);
> This list code looks strange.
>
Won't need that after making the required changes.
>> +
>> + /* Iterate through the notes and retrieve the required note */
>> + list_for_each(pos, sdt_notes) {
>> + tmp = list_entry(pos, struct sdt_note, note_list);
>> + if (!strcmp(key->name, tmp->name) &&
>> + !strcmp(key->provider, tmp->provider)) {
>> + adjust_note_addr(tmp, key, elf);
>> + ret = 0;
>> + break;
>> + }
>> + }
>> + if (ret)
>> + printf("%%%s:%s not found!\n", key->provider, key->name);
>> +
>> +out_end:
>> + elf_end(elf);
>> +out_close:
>> + close(fd);
>> +out_ret:
>> + return ret;
>> +}
>> +
>> void symbol__elf_init(void)
>> {
>> elf_version(EV_CURRENT);
>> diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
>> index 037185c..0b0545b 100644
>> --- a/tools/perf/util/symbol.h
>> +++ b/tools/perf/util/symbol.h
>> @@ -260,9 +260,12 @@ void __map_groups__fixup_end(struct map_groups *mg, enum map_type type);
>>
>> /* Specific to SDT notes */
>> int get_sdt_note_list(struct list_head *head, const char *target);
>> +int search_sdt_note(struct sdt_note *key, struct list_head *head,
>> + const char *target);
>>
>> #define SDT_NOTE_TYPE 3
>> #define NOTE_SCN ".note.stapsdt"
>> #define SDT_NOTE_NAME "stapsdt"
>> +#define SDT_BASE ".stapsdt.base"
> What about SDT_BASE_SCN ?
Seems better.
--
Thanks
Hemant
--
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/
Powered by blists - more mailing lists