Re: [PATCH 1/5] perf config: Move perf_config__set_variable() to util/config.c

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] perf config: data race on config_file_name causing NULL dereference in bad_config()
- [Medium] perf config: memory leak in perf_home_perfconfig() and perf_config__init() due to concurrent access
--

commit 6feab70cc124ed967d7949683aa5b69b49d7b578
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf config: Move perf_config__set_variable() to util/config.c

Move perf_config__set_variable() out of the perf config builtin so
that opt-in features can persist their choice from outside it. All config
file access shares the static parser state and can now run on more than
one thread, so parsing and rewriting are serialized with a mutex.

> diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
> index 8fe43b032e9af..b33964be0c14a 100644
> --- a/tools/perf/util/config.c
> +++ b/tools/perf/util/config.c
> @@ -549,11 +551,26 @@ int perf_default_config(const char *var, const char *value,
>  	return 0;
>  }
>  
> +/*
> + * Serialize config file access: parsing and rewriting share the static
> + * parser state and can run on more than one thread, the debuginfod
> + * fetch writing core.debuginfod=false while perf top reads it.
> + */
> +static pthread_mutex_t config_mutex = PTHREAD_MUTEX_INITIALIZER;
> +

[Severity: Medium]
Since this patch enables multithreaded access to the configuration state,
can concurrent calls to lockless lazy initialization functions leak memory?

For example, in perf_home_perfconfig():

tools/perf/util/config.c:perf_home_perfconfig() {
	static const char *config;
	static bool failed;

	if (failed || config)
		return config;

	config = home_perfconfig();
	...
}

If two threads evaluate config as NULL simultaneously, could they both call
home_perfconfig() and overwrite the first thread's allocation, leaking the
allocated config path string?

Similarly, in perf_config__init():

tools/perf/util/config.c:perf_config__init() {
	if (config_set == NULL)
		config_set = perf_config_set__new();
	...
}

Could concurrent threads duplicate the parsing and leak the resulting global
configuration set structure?

[ ... ]

> @@ -783,8 +819,15 @@ static int collect_config(const char *var, const char *value,
>  int perf_config_set__collect(struct perf_config_set *set, const char *file_name,
>  			     const char *var, const char *value)
>  {
> +	int ret;
> +
> +	pthread_mutex_lock(&config_mutex);
>  	config_file_name = file_name;
> -	return collect_config(var, value, set);
> +	ret = collect_config(var, value, set);
> +		/* Don't leave the static parser state pointing at the caller's buffer. */
> +	config_file_name = NULL;
> +	pthread_mutex_unlock(&config_mutex);
> +	return ret;
>  }

[Severity: Medium]
Does this temporary modification of config_file_name introduce a data race
with unlocked readers?

While config_mutex is held here, another thread might execute perf_config()
and call bad_config() concurrently:

tools/perf/util/config.c:bad_config() {
	if (config_file_name)
		pr_warning("bad config value for '%s' in %s, ignoring...\n",
			   name, config_file_name);
	...
}

Since bad_config() does not hold config_mutex, if thread A evaluates
config_file_name as non-NULL, and thread B immediately sets it to NULL in
perf_config_set__collect(), could thread A then pass a NULL pointer to the
%s format specifier in pr_warning()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929212246.2532021-1-acme@kernel.org?part=1