Re: [PATCH v7 5/9] perf config: Serialize config file access with a mutex

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] perf: DO_ONCE lacks memory barriers causing double-checked locking race
--

commit 0b6c61f1a2ea254be797a11efae079eb0c53f03d
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf config: Serialize config file access with a mutex

This commit serializes perf config file access using mutexes to support
concurrent reads and writes from background features. It also replaces the
lazy initialization of system_path() and home_perfconfig() with DO_ONCE() to
prevent string leaks when accessed by multiple threads.

> diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
> index 6e0ff8a9140cc..287401f159e66 100644
> --- a/tools/perf/util/config.c
> +++ b/tools/perf/util/config.c

[ ... ]

> @@ -565,21 +571,24 @@ static int perf_config_from_file(config_fn_t fn, const char *filename, void *dat
>  		fclose(f);
>  		config_file_name = NULL;
>  	}
> +	mutex_unlock(&config_mutex);
>  	return ret;
>  }
>  
> -const char *perf_etc_perfconfig(void)
> -{
> -	static const char *system_wide;
> +/* system_path() allocates, so it is computed once. */
> +static const char *etc_perfconfig;
>  
> -	if (!system_wide)
> -		/*
> -		 * ETC_PERFCONFIG is absolute, so its unresolved path is
> -		 * the same string, better than the callers crashing.
> -		 */
> -		system_wide = system_path(ETC_PERFCONFIG) ?: ETC_PERFCONFIG;
> +static void perf_etc_perfconfig__init(void)
> +{
> +	etc_perfconfig = system_path(ETC_PERFCONFIG);
> +	if (!etc_perfconfig)
> +		etc_perfconfig = ETC_PERFCONFIG;
> +}
>  
> -	return system_wide;
> +const char *perf_etc_perfconfig(void)
> +{
> +	DO_ONCE(perf_etc_perfconfig__init);

[Severity: High]
Does this use of DO_ONCE() introduce a double-checked locking race?

The DO_ONCE() macro in tools/perf/util/mutex.h implements a lockless check
of the ___done flag without memory barriers:

        if (!___done) { \
                mutex_lock(&___once_lock); \
                if (!___done) { \
                        fn(__VA_ARGS__); \
                        ___done = true;

Because of the missing barriers (such as smp_load_acquire and
smp_store_release), a second thread can observe ___done == true out of
order, before the stores initializing etc_perfconfig become globally
visible.

Can this result in returning an uninitialized NULL value to callers relying
on a valid string?

> +	return etc_perfconfig;
> +}
>  
>  static int perf_env_bool(const char *k, int def)

[ ... ]

> @@ -637,19 +646,18 @@ static char *home_perfconfig(void)
>  	return NULL;
>  }
>  
> -const char *perf_home_perfconfig(void)
> -{
> -	static const char *config;
> -	static bool failed;
> -
> -	if (failed || config)
> -		return config;
> +/* home_perfconfig() allocates and warns, so it is computed once. */
> +static const char *home_config;
>  
> -	config = home_perfconfig();
> -	if (!config)
> -		failed = true;
> +static void perf_home_perfconfig__init(void)
> +{
> +	home_config = home_perfconfig();
> +}
>  
> -	return config;
> +const char *perf_home_perfconfig(void)
> +{
> +	DO_ONCE(perf_home_perfconfig__init);

[Severity: High]
Could this similarly risk returning a NULL pointer due to the non-atomic
DO_ONCE() implementation described above?

> +	return home_config;
> +}
>  
>  static struct perf_config_section *find_section(struct list_head *sections,

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