Re: [PATCH 03/15] 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 loss due to non-atomic read-modify-write in set_variable
- [Medium] perf config: memory leak due to lockless perf_etc_perfconfig() initialization
--

commit 836076e44736c46148a07294c62bef2cd50992f3
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

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

This commit moves perf_config__set_variable() to util/config.c so that other
utilities can access it. It also serializes config parsing and rewriting
with a mutex to support multithreaded access.

> diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
> --- a/tools/perf/util/config.c
> +++ b/tools/perf/util/config.c
[ ... ]
> @@ -776,8 +788,13 @@ 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;

[Severity: Medium]
This is a pre-existing issue, but does assigning file_name to the static
global config_file_name create a stack use-after-free risk?

In perf_config__set_variable(), the stack array path is used as the filename
and passed into this function:

perf_config__set_variable() {
    char path[PATH_MAX];
    ...
    const char *config_filename = ... ?: mkpath(path, ...);
    perf_config_set__collect(..., config_filename, ...);
}

If the static pointer config_file_name isn't cleared after collecting, could
a subsequent config parsing failure in bad_config() dereference this dangling
stack pointer and cause a crash or output garbage?

> -	return collect_config(var, value, set);
> +	ret = collect_config(var, value, set);
> +	pthread_mutex_unlock(&config_mutex);
> +	return ret;
>  }
[ ... ]
> @@ -876,6 +893,83 @@ void perf_config__exit(void)
[ ... ]
> +int perf_config__set_variable(const char *var, const char *value)
> +{
> +	char path[PATH_MAX];
> +	char *user_config = mkpath(path, sizeof(path), "%s/.perfconfig", getenv("HOME"));
> +	const char *config_filename = config_exclusive_filename ?: user_config;
> +	/*
> +	 * When rewriting the system wide file (e.g. with
> +	 * PERF_CONFIG=/etc/perfconfig) all entries are marked as coming from it
> +	 * and must be kept, or the file would be truncated down to its header.
> +	 */
> +	bool system_config = strcmp(config_filename, perf_etc_perfconfig()) == 0;

[Severity: Medium]
Can calling perf_etc_perfconfig() concurrently here lead to a memory leak?

Because perf_etc_perfconfig() checks !system_wide without holding
config_mutex, multiple threads could bypass the check simultaneously and
allocate redundant path strings via system_path(). This could cause all but
the last allocated string to be leaked.

> +	struct perf_config_set *set;
> +	int ret = -1;
> +
> +	set = perf_config_set__new();
> +	if (!set)
> +		goto out_err;
> +
> +	if (perf_config_set__collect(set, config_filename, var, value) < 0) {
> +		pr_err("Failed to add '%s=%s'\n", var, value);
> +		goto out_err;
> +	}
> +
> +	if (perf_config_set__write(set, config_filename, system_config) < 0) {

[Severity: Medium]
Can concurrent configuration updates overwrite each other here, causing
data loss?

The sequence of perf_config_set__new(), perf_config_set__collect(), and
perf_config_set__write() drops the config_mutex between each function call.
This appears to break the atomicity of the read-modify-write cycle when
multiple threads invoke perf_config__set_variable() at the same time.

> +		pr_err("Failed to set the configs on %s\n", config_filename);
> +		goto out_err;
> +	}
> +
> +	ret = 0;
> +out_err:
> +	perf_config_set__delete(set);
> +	return ret;
> +}

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