Re: [PATCH 3/8] perf symbol: Fall back to fetching the vmlinux by build ID

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Race condition in debuginfod vmlinux fetch discards successful downloads
--

commit 808ca5b7ae0805113b62f51bed90e6469e600d15
Author: Arnaldo Carvalho de Melo [off-list ref]
perf symbol: Fall back to fetching the vmlinux by build ID

This commit adds a fallback mechanism to fetch the vmlinux by build ID using
debuginfod when local kernel symbols cannot be found. This allows kernel
samples to be correctly annotated even when the profile is processed on a
machine with a different kernel or restricted kallsyms access.

[ ... ]

[ ... ]

+		/*
+		 * dso__load() holds dso->lock while it calls us, and the
+		 * fetch below can take a long time, blocked on the network
+		 * or on the terminal, waiting for the user: do it with the
+		 * lock dropped, as dso__debuginfo() does for the debuginfo
+		 * of a DSO, so that the threads that need this dso don't get
+		 * stuck behind a server round trip.  Nothing of the dso is
+		 * touched by the fetch, the symbols are loaded with the lock
+		 * held again, and only if the fetch brought a file back.
+		 */
+		mutex_unlock(dso__lock(dso));
+		err = dso__fetch_vmlinux_build_id(dso, &fetched_path);
+		mutex_lock(dso__lock(dso));
+
+		if (err) {
+			zfree(&fetched_path);
+		} else if (dso__loaded(dso)) {

[Severity: Medium]
Does this logic safely handle concurrent fetches where one thread fails? 

Because dso__load() unconditionally sets dso__loaded() even on failure, this
could discard a successfully fetched vmlinux if a concurrent thread previously
failed.

For example, if Thread A and Thread B concurrently attempt to load kernel 
symbols via dso__load():
1. Both drop the lock to fetch via debuginfod.
2. Thread A fails its fetch, reacquires the lock, returns an error, and its
   caller (dso__load()) unconditionally sets dso__loaded(dso) = true before
   releasing the lock.
3. Thread B succeeds in fetching, reacquires the lock, and checks
   dso__loaded(dso).

Seeing it is true, wouldn't Thread B incorrectly assume Thread A successfully
loaded the symbols, discard its own fetched file, and return success?

Would it be better to use dso__has_symbols(dso) instead of dso__loaded(dso)
here to correctly detect a successful concurrent load?

+			/*
+			 * Somebody else got the symbols for this dso while
+			 * the lock was dropped for the fetch, use those
+			 * instead of loading the file that came back a
+			 * second time.
+			 */
+			pr_debug("%s was loaded while its vmlinux was being fetched, using it\n",
+				 dso__name(dso));
+			zfree(&fetched_path);
+			err = 1;
+		} else {
+			/* Takes ownership of 'fetched_path' even when it fails */
+			err = dso__load_vmlinux(dso, map, fetched_path, true);
+		}

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