linux-kernel - Re: [PATCH v1] perf tracepoint: Don't scan all tracepoints to test if one exists

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] [day] [month] [year] [list]

Message-ID: <Zj4tVDdY3CzU7KZM@x1>
Date: Fri, 10 May 2024 11:21:08 -0300
From: Arnaldo Carvalho de Melo <acme@...nel.org>
To: Ian Rogers <irogers@...gle.com>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
	Namhyung Kim <namhyung@...nel.org>,
	Mark Rutland <mark.rutland@....com>,
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
	Jiri Olsa <jolsa@...nel.org>,
	Adrian Hunter <adrian.hunter@...el.com>,
	Kan Liang <kan.liang@...ux.intel.com>,
	linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1] perf tracepoint: Don't scan all tracepoints to test
 if one exists

On Wed, May 08, 2024 at 11:05:41PM -0700, Ian Rogers wrote:
> In is_valid_tracepoint, rather than scanning
> "/sys/kernel/tracing/events/*/*" skipping any path where
> "/sys/kernel/tracing/events/*/*/id" doesn't exist, and then testing if
> "*:*" matches the tracepoint name, just use the given tracepoint name
> replace the ':' with '/' and see if the id file exists. This turns a
> nested directory search into a single file available test.
> 
> Rather than return 1 for valid and 0 for invalid, return true and
> false.
> 
> Signed-off-by: Ian Rogers <irogers@...gle.com>
> ---
>  tools/perf/util/tracepoint.c | 53 ++++++++++++++----------------------
>  tools/perf/util/tracepoint.h |  3 +-
>  2 files changed, 23 insertions(+), 33 deletions(-)
> 
> diff --git a/tools/perf/util/tracepoint.c b/tools/perf/util/tracepoint.c
> index 92dd8b455b90..cca6808f7951 100644
> --- a/tools/perf/util/tracepoint.c
> +++ b/tools/perf/util/tracepoint.c
> @@ -4,10 +4,12 @@
>  #include <errno.h>
>  #include <fcntl.h>
>  #include <stdio.h>
> +#include <stdlib.h>
>  #include <sys/param.h>
>  #include <unistd.h>
>  
>  #include <api/fs/tracing_path.h>
> +#include "fncache.h"
>  
>  int tp_event_has_id(const char *dir_path, struct dirent *evt_dir)
>  {
> @@ -26,39 +28,26 @@ int tp_event_has_id(const char *dir_path, struct dirent *evt_dir)
>  /*
>   * Check whether event is in <debugfs_mount_point>/tracing/events
>   */
> -int is_valid_tracepoint(const char *event_string)
> +bool is_valid_tracepoint(const char *event_string)
>  {
> -	DIR *sys_dir, *evt_dir;
> -	struct dirent *sys_dirent, *evt_dirent;
> -	char evt_path[MAXPATHLEN];
> -	char *dir_path;
> -
> -	sys_dir = tracing_events__opendir();
> -	if (!sys_dir)
> -		return 0;
> +	char *dst, *path = malloc(strlen(event_string) + 4); /* Space for "/id\0". */
> +	const char *src;
> +	bool have_file;
>  
> -	for_each_subsystem(sys_dir, sys_dirent) {
> -		dir_path = get_events_file(sys_dirent->d_name);
> -		if (!dir_path)
> -			continue;
> -		evt_dir = opendir(dir_path);
> -		if (!evt_dir)
> -			goto next;
> -
> -		for_each_event(dir_path, evt_dir, evt_dirent) {
> -			snprintf(evt_path, MAXPATHLEN, "%s:%s",
> -				 sys_dirent->d_name, evt_dirent->d_name);
> -			if (!strcmp(evt_path, event_string)) {
> -				closedir(evt_dir);
> -				put_events_file(dir_path);
> -				closedir(sys_dir);
> -				return 1;
> -			}
> -		}
> -		closedir(evt_dir);
> -next:
> -		put_events_file(dir_path);
> +	if (!path) {
> +		/* Conservatively return false if memory allocation failed. */
> +		return false;
>  	}
> -	closedir(sys_dir);
> -	return 0;
> +	/* Copy event_string replacing the ':' with '/'. */
> +	for (src = event_string, dst = path; *src; src++, dst++)
> +		*dst = (*src == ':') ? '/' : *src;
> +	/* Add "/id\0". */
> +	memcpy(dst, "/id", 4);
> +
> +	dst = get_events_file(path);
> +	if (dst)
> +		have_file = file_available(dst);

If dst == NULL have_file isn't initialized?

> +	free(dst);
> +	free(path);
> +	return have_file;
>  }
> diff --git a/tools/perf/util/tracepoint.h b/tools/perf/util/tracepoint.h
> index c4a110fe87d7..65ccb01fc312 100644
> --- a/tools/perf/util/tracepoint.h
> +++ b/tools/perf/util/tracepoint.h
> @@ -4,6 +4,7 @@
>  
>  #include <dirent.h>
>  #include <string.h>
> +#include <stdbool.h>
>  
>  int tp_event_has_id(const char *dir_path, struct dirent *evt_dir);
>  
> @@ -20,6 +21,6 @@ int tp_event_has_id(const char *dir_path, struct dirent *evt_dir);
>  		    (strcmp(sys_dirent->d_name, ".")) &&	\
>  		    (strcmp(sys_dirent->d_name, "..")))
>  
> -int is_valid_tracepoint(const char *event_string);
> +bool is_valid_tracepoint(const char *event_string);
>  
>  #endif /* __PERF_TRACEPOINT_H */
> -- 
> 2.45.0.rc1.225.g2a3ae87e7f-goog