linux-kernel - Re: [PATCH v2 1/3] perf/sdt : Listing of SDT markers by perf

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: <87d2d24kui.fsf@tassilo.jf.intel.com>
Date:	Fri, 18 Jul 2014 10:50:45 -0700
From:	Andi Kleen <andi@...stfloor.org>
To:	Hemant Kumar <hemant@...ux.vnet.ibm.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,
	masami.hiramatsu.pt@...achi.com, aravinda@...ux.vnet.ibm.com,
	penberg@....fi
Subject: Re: [PATCH v2 1/3] perf/sdt : Listing of SDT markers by perf

Hemant Kumar <hemant@...ux.vnet.ibm.com> writes:

First I should say supporting these probes is very useful. Thanks for
working on this.

> +
> +#define SDT_CACHE_DIR "/var/cache/perf/"

This requires running perf as root, right?

It would be better to use the $HOME cache dir, like the recent JSON patches.

> +#define SDT_CACHE "perf-sdt.cache"
> +#define SDT_CACHE_TMP "perf-sdt.cache.tmp"
> +
> +#define DELIM ':'
> +
> +struct path_list {
> +	char path[PATH_MAX];
> +	struct list_head list;
> +} execs;
> +
> +/* Write operation for cache */
> +static void write_cache(FILE *cache, char *buffer)
> +{
> +	fprintf(cache, "%s", buffer);
> +}
> +

The function seems redundant.


> +/*
> + * get_sdt_note_info() is the function actually responsible for
> + * flushing the SDT notes info into the "cache" file or to the
> + * stdout if "cache" points to NULL. Also, this function finds out
> + * the build-id of an ELF to be written into "cache".
> + */
> +static void get_sdt_note_info(struct list_head *start, const char *target,
> +			      FILE *cache)
> +{
> +	struct sdt_note *pos;
> +	u8 build_id[BUILD_ID_SIZE];
> +	char sbuild_id[BUILD_ID_SIZE * 2 + 1];
> +	char buffer[2 * PATH_MAX];
> +
> +	if (list_empty(start))
> +		return;
> +
> +	/* Read the build id of the file */
> +	if (filename__read_build_id(target, &build_id,
> +				    sizeof(build_id)) < 0) {
> +		pr_debug("Couldn't read build-id in %s\n", target);

pr_info ?

> +
> +/*
> + * Finds out the libraries present in a system as shown by the command
> + * "ldconfig --print-cache". Uses "=>" and '/' to find out the start of a
> + * dso path.
> + */

This seems like a hack. How would that handle chroot, containers etc. ?

> +static inline void append_path(char *path, struct list_head *list)
> +{
> +	char *res_path = NULL;
> +	struct path_list *tmp = NULL;
> +
> +	res_path = (char *)malloc(sizeof(char) * PATH_MAX);
> +
> +	if (!res_path)
> +		return;
> +
> +	memset(res_path, '\0', PATH_MAX);
> +
> +	if (realpath(path, res_path) && !is_present_in_list(list, res_path)) {


O^2 algorithm ?

> +/*
> + * Obtain the list of paths from the PATH env variable
> + */

Same as above. This probably needs to be more configurable to handle
more ways to find binaries.


-Andi

-- 
ak@...ux.intel.com -- Speaking for myself only
--
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/