Re: [PATCH v4 07/17] perf vdso: Fix unlocked __dsos__add() and race in __machine__addnew_vdso()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf vdso: unlocked vdso_info and get_file() race in machine__findnew_vdso()
--

commit a9b1f4c5120b2f5d63a8c9e9ad23dc250184a018
Author: Ian Rogers <irogers@google.com>

perf vdso: Fix unlocked __dsos__add() and race in __machine__addnew_vdso()

This commit acquires machine->dsos.lock in write mode in
__machine__addnew_vdso(), double-checks __dsos__find() to avoid inserting
duplicate VDSO entries when racing with another thread, and sets the DSO
long name before calling __dsos__add().

> diff --git a/tools/perf/util/vdso.c b/tools/perf/util/vdso.c
> index c12f5d8c4bf69..dca4f683be9b7 100644
> --- a/tools/perf/util/vdso.c
> +++ b/tools/perf/util/vdso.c
> @@ -127,13 +127,26 @@ void machine__exit_vdso(struct machine *machine)
>  static struct dso *__machine__addnew_vdso(struct machine *machine, const char *short_name,
>  					  const char *long_name)
>  {
> -	struct dso *dso;
> +	struct dso *dso, *existing;
> +
> +	down_write(&machine->dsos.lock);
> +	existing = __dsos__find(&machine->dsos, short_name, true, /*write_locked=*/true);
> +	if (existing) {
> +		up_write(&machine->dsos.lock);
> +		return existing;
> +	}

[Severity: Medium]
Can concurrent callers of machine__findnew_vdso() still race on
machine->vdso_info and vdso_file before reaching __machine__addnew_vdso()?

The callers of __machine__addnew_vdso() are machine__findnew_vdso() and
__machine__findnew_compat():

tools/perf/util/vdso.c:machine__findnew_vdso() {
	if (!machine->vdso_info)
		machine->vdso_info = vdso_info__new();

	vdso_info = machine->vdso_info;
	if (!vdso_info)
		return NULL;
	...
	file = get_file(&vdso_info->vdso);
	if (!file)
		return NULL;

	return __machine__addnew_vdso(machine, DSO__NAME_VDSO, file);
}

tools/perf/util/vdso.c:__machine__findnew_compat() {
	...
	file_name = vdso__get_compat_file(vdso_file);
	if (!file_name)
		return NULL;

	return __machine__addnew_vdso(machine, vdso_file->dso_name, file_name);
}

When two threads enter machine__findnew_vdso() while machine->vdso_info is
NULL, both can call vdso_info__new() and overwrite machine->vdso_info,
leaking the first struct vdso_info allocation and any temporary file it
creates.

Even if both threads see the same machine->vdso_info while vdso_file->found
is still false, both can enter get_file() (or vdso__get_compat_file())
before acquiring machine->dsos.lock:

tools/perf/util/vdso.c:get_file() {
	if (vdso_file->found)
		return vdso_file->temp_file_name;
	...
	fd = mkstemp(vdso_file->temp_file_name);
	if (fd < 0)
		goto out;
	...
 out:
	free(buf);

	vdso_file->found = (vdso != NULL);
	vdso_file->error = !vdso_file->found;
	return vdso;
}

Because mkstemp() mutates vdso_file->temp_file_name in place (replacing the
trailing "XXXXXX"), the second thread's mkstemp() call on the same buffer
fails with EINVAL, sets vdso_file->found = false and vdso_file->error =
true, and returns NULL before ever reaching the double-check in
__machine__addnew_vdso().

Should the machine->vdso_info initialization and temporary file creation in
get_file() and vdso__get_compat_file() also be synchronized under the lock?

>  	dso = dso__new(short_name);
>  	if (dso != NULL) {
> -		__dsos__add(&machine->dsos, dso);
> +		/*
> +		 * Set the long name before adding to machine->dsos so that
> +		 * __dsos__add() inserts it in sorted order without needing to
> +		 * re-take machine->dsos.lock.
> +		 */
>  		dso__set_long_name(dso, long_name, false);
> +		__dsos__add(&machine->dsos, dso);
>  	}
> +	up_write(&machine->dsos.lock);
>  
>  	return dso;
>  }

完美结束，Wait—remove non-ASCII text at the end! Let me make sure I don't output anything after the blank line.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016221700.1282659-1-irogers@google.com?part=7