linux-kernel - Re: [PATCH v2 1/5] perf/sdt: ELF support for SDT

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: <5430FEA9.3090405@linux.vnet.ibm.com>
Date:	Sun, 05 Oct 2014 13:47:45 +0530
From:	Hemant Kumar <hemant@...ux.vnet.ibm.com>
To:	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
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, namhyung@...nel.org,
	aravinda@...ux.vnet.ibm.com, penberg@....fi
Subject: Re: [PATCH v2 1/5] perf/sdt: ELF support for SDT


On 10/03/2014 05:09 PM, Masami Hiramatsu wrote:
> (2014/10/01 11:47), Hemant Kumar wrote:
>> This patch serves as the basic support to identify and list SDT events in binaries.
>> When programs containing SDT markers are compiled, gcc with the help of assembler
>> directives identifies them and places them in the section ".note.stapsdt". To find
>> these markers from the binaries, one needs to traverse through this section and
>> parse the relevant details like the name, type and location of the marker. Also,
>> the original location could be skewed due to the effect of prelinking. If that is
>> the case, the locations need to be adjusted.
>>
>> The functions in this patch open a given ELF, find out the SDT section, parse the
>> relevant details, adjust the location (if necessary) and populate them in a list.
>>
>> Made the necessary changes as suggested by Namhyung Kim.
> Looks almost good!
> I have some comments, please see below.

Ok.

>> Signed-off-by: Hemant Kumar <hemant@...ux.vnet.ibm.com>
>> ---
>>   tools/perf/util/symbol-elf.c |  207 ++++++++++++++++++++++++++++++++++++++++++
>>   tools/perf/util/symbol.h     |   19 ++++
>>   2 files changed, 226 insertions(+)
>>
>> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
>> index 2a92e10..9702167 100644
>> --- a/tools/perf/util/symbol-elf.c
>> +++ b/tools/perf/util/symbol-elf.c
>> @@ -1672,6 +1672,213 @@ void kcore_extract__delete(struct kcore_extract *kce)
>>   	unlink(kce->extract_filename);
>>   }
>>   
>> +/*
>> + * populate_sdt_note() : Responsible for parsing the section .note.stapsdt and
>> + * after adjusting the note's location, returns that to the calling functions.
>> + */
>> +static int populate_sdt_note(Elf **elf, const char *data, size_t len, int type,
>> +			     struct sdt_note **note)
>> +{
>> +	const char *provider, *name;
>> +	struct sdt_note *tmp = NULL;
>> +	GElf_Ehdr ehdr;
>> +	GElf_Addr base_off = 0;
>> +	GElf_Shdr shdr;
>> +	int ret = -1;
> -1 is not an error code. maybe, -EINVAL is better.

Sure, will change this to EINVAL.

>> +	int i;
>> +
>> +	union {
>> +		Elf64_Addr a64[3];
>> +		Elf32_Addr a32[3];
>> +	} buf;
>> +
>> +	Elf_Data dst = {
>> +		.d_buf = &buf, .d_type = ELF_T_ADDR, .d_version = EV_CURRENT,
>> +		.d_size = gelf_fsize((*elf), ELF_T_ADDR, 3, EV_CURRENT),
>> +		.d_off = 0, .d_align = 0
>> +	};
>> +	Elf_Data src = {
>> +		.d_buf = (void *) data, .d_type = ELF_T_ADDR,
>> +		.d_version = EV_CURRENT, .d_size = dst.d_size, .d_off = 0,
>> +		.d_align = 0
>> +	};
>> +
>> +	/* Check the type of each of the notes */
>> +	if (type != SDT_NOTE_TYPE)
>> +		goto out_err;
>> +
>> +	tmp = (struct sdt_note *)calloc(1, sizeof(struct sdt_note));
>> +	if (!tmp) {
>> +		ret = -ENOMEM;
>> +		goto out_err;
>> +	}
>> +
>> +	INIT_LIST_HEAD(&tmp->note_list);
>> +
>> +	if (len < dst.d_size + 3)
>> +		goto out_free_note;
>> +
>> +	/* Translation from file representation to memory representation */
>> +	if (gelf_xlatetom(*elf, &dst, &src,
>> +			  elf_getident(*elf, NULL)[EI_DATA]) == NULL)
>> +		printf("gelf_xlatetom : %s\n", elf_errmsg(-1));
> pr_err? and shouldn't it goes to out_free_note?

Yes it should go to out_free_note. Will make the change.

>> [...]
>>
>>

Thank You for reviewing this patch.

-- 
Thanks,
Hemant Kumar

--
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/