Commit 5ac22c35 authored by Ian Rogers's avatar Ian Rogers Committed by Namhyung Kim
Browse files

perf dso: Use lock annotations to fix asan deadlock 



dso__list_del with address sanitizer and/or reference count checking
will call dso__put that can call dso__data_close reentrantly trying to
lock the dso__data_open_lock and deadlocking. Switch from pthread
mutexes to perf's mutex so that lock checking is performed in debug
builds. Add lock annotations that diagnosed the problem. Release the
dso__data_open_lock around the dso__put to avoid the deadlock.

Change the declaration of dso__data_get_fd to return a boolean,
indicating the fd is valid and the lock is held, to make it compatible
with the thread safety annotations as a try lock.

Signed-off-by: default avatarIan Rogers <irogers@google.com>
Link: https://lore.kernel.org/r/20250318043151.137973-3-irogers@google.com


Signed-off-by: default avatarNamhyung Kim <namhyung@kernel.org>
parent c5ebf3a2

tools/perf/tests/dso-data.c

+2 −2
Original line number Diff line number Diff line
@@ -106,9 +106,9 @@ struct test_data_offset offsets[] = { 
/* move it from util/dso.c for compatibility */

static int dso__data_fd(struct dso *dso, struct machine *machine)

{

	int fd = dso__data_get_fd(dso, machine);

	int fd = -1;



	if (fd >= 0)

	if (dso__data_get_fd(dso, machine, &fd))

		dso__data_put_fd(dso);



	return fd;

tools/perf/util/dso.c

+47 −27
Original line number Diff line number Diff line
@@ -493,11 +493,25 @@ void dso__set_module_info(struct dso *dso, struct kmod_path *m, 
/*

 * Global list of open DSOs and the counter.

 */

struct mutex _dso__data_open_lock;

static LIST_HEAD(dso__data_open);

static long dso__data_open_cnt;

static pthread_mutex_t dso__data_open_lock = PTHREAD_MUTEX_INITIALIZER;

static long dso__data_open_cnt GUARDED_BY(_dso__data_open_lock);



static void dso__list_add(struct dso *dso)

static void dso__data_open_lock_init(void)

{

	mutex_init(&_dso__data_open_lock);

}



static struct mutex *dso__data_open_lock(void) LOCK_RETURNED(_dso__data_open_lock)

{

	static pthread_once_t data_open_lock_once = PTHREAD_ONCE_INIT;



	pthread_once(&data_open_lock_once, dso__data_open_lock_init);



	return &_dso__data_open_lock;

}



static void dso__list_add(struct dso *dso) EXCLUSIVE_LOCKS_REQUIRED(_dso__data_open_lock)

{

	list_add_tail(&dso__data(dso)->open_entry, &dso__data_open);

#ifdef REFCNT_CHECKING
@@ -508,11 +522,13 @@ static void dso__list_add(struct dso *dso) 
	dso__data_open_cnt++;

}



static void dso__list_del(struct dso *dso)

static void dso__list_del(struct dso *dso) EXCLUSIVE_LOCKS_REQUIRED(_dso__data_open_lock)

{

	list_del_init(&dso__data(dso)->open_entry);

#ifdef REFCNT_CHECKING

	mutex_unlock(dso__data_open_lock());

	dso__put(dso__data(dso)->dso);

	mutex_lock(dso__data_open_lock());

#endif

	WARN_ONCE(dso__data_open_cnt <= 0,

		  "DSO data fd counter out of bounds.");
@@ -521,7 +537,7 @@ static void dso__list_del(struct dso *dso) 


static void close_first_dso(void);



static int do_open(char *name)

static int do_open(char *name) EXCLUSIVE_LOCKS_REQUIRED(_dso__data_open_lock)

{

	int fd;

	char sbuf[STRERR_BUFSIZE];
@@ -548,6 +564,7 @@ char *dso__filename_with_chroot(const struct dso *dso, const char *filename) 
}



static int __open_dso(struct dso *dso, struct machine *machine)

	EXCLUSIVE_LOCKS_REQUIRED(_dso__data_open_lock)

{

	int fd = -EINVAL;

	char *root_dir = (char *)"";
@@ -613,6 +630,7 @@ static void check_data_close(void); 
 * list/count of open DSO objects.

 */

static int open_dso(struct dso *dso, struct machine *machine)

	EXCLUSIVE_LOCKS_REQUIRED(_dso__data_open_lock)

{

	int fd;

	struct nscookie nsc;
@@ -638,7 +656,7 @@ static int open_dso(struct dso *dso, struct machine *machine) 
	return fd;

}



static void close_data_fd(struct dso *dso)

static void close_data_fd(struct dso *dso) EXCLUSIVE_LOCKS_REQUIRED(_dso__data_open_lock)

{

	if (dso__data(dso)->fd >= 0) {

		close(dso__data(dso)->fd);
@@ -655,12 +673,12 @@ static void close_data_fd(struct dso *dso) 
 * Close @dso's data file descriptor and updates

 * list/count of open DSO objects.

 */

static void close_dso(struct dso *dso)

static void close_dso(struct dso *dso) EXCLUSIVE_LOCKS_REQUIRED(_dso__data_open_lock)

{

	close_data_fd(dso);

}



static void close_first_dso(void)

static void close_first_dso(void) EXCLUSIVE_LOCKS_REQUIRED(_dso__data_open_lock)

{

	struct dso_data *dso_data;

	struct dso *dso;
@@ -705,7 +723,7 @@ void reset_fd_limit(void) 
	fd_limit = 0;

}



static bool may_cache_fd(void)

static bool may_cache_fd(void) EXCLUSIVE_LOCKS_REQUIRED(_dso__data_open_lock)

{

	if (!fd_limit)

		fd_limit = get_fd_limit();
@@ -721,7 +739,7 @@ static bool may_cache_fd(void) 
 * for opened dso file descriptors. The limit is half

 * of the RLIMIT_NOFILE files opened.

*/

static void check_data_close(void)

static void check_data_close(void) EXCLUSIVE_LOCKS_REQUIRED(_dso__data_open_lock)

{

	bool cache_fd = may_cache_fd();
@@ -737,12 +755,13 @@ static void check_data_close(void) 
 */

void dso__data_close(struct dso *dso)

{

	pthread_mutex_lock(&dso__data_open_lock);

	mutex_lock(dso__data_open_lock());

	close_dso(dso);

	pthread_mutex_unlock(&dso__data_open_lock);

	mutex_unlock(dso__data_open_lock());

}



static void try_to_open_dso(struct dso *dso, struct machine *machine)

	EXCLUSIVE_LOCKS_REQUIRED(_dso__data_open_lock)

{

	enum dso_binary_type binary_type_data[] = {

		DSO_BINARY_TYPE__BUILD_ID_CACHE,
@@ -784,25 +803,27 @@ static void try_to_open_dso(struct dso *dso, struct machine *machine) 
 * returns file descriptor.  It should be paired with

 * dso__data_put_fd() if it returns non-negative value.

 */

int dso__data_get_fd(struct dso *dso, struct machine *machine)

bool dso__data_get_fd(struct dso *dso, struct machine *machine, int *fd)

{

	*fd = -1;

	if (dso__data(dso)->status == DSO_DATA_STATUS_ERROR)

		return -1;

		return false;



	if (pthread_mutex_lock(&dso__data_open_lock) < 0)

		return -1;

	mutex_lock(dso__data_open_lock());



	try_to_open_dso(dso, machine);



	if (dso__data(dso)->fd < 0)

		pthread_mutex_unlock(&dso__data_open_lock);

	*fd = dso__data(dso)->fd;

	if (*fd >= 0)

		return true;



	return dso__data(dso)->fd;

	mutex_unlock(dso__data_open_lock());

	return false;

}



void dso__data_put_fd(struct dso *dso __maybe_unused)

{

	pthread_mutex_unlock(&dso__data_open_lock);

	mutex_unlock(dso__data_open_lock());

}



bool dso__data_status_seen(struct dso *dso, enum dso_data_status_seen by)
@@ -954,7 +975,7 @@ static ssize_t file_read(struct dso *dso, struct machine *machine, 
{

	ssize_t ret;



	pthread_mutex_lock(&dso__data_open_lock);

	mutex_lock(dso__data_open_lock());



	/*

	 * dso__data(dso)->fd might be closed if other thread opened another
@@ -970,7 +991,7 @@ static ssize_t file_read(struct dso *dso, struct machine *machine, 


	ret = pread(dso__data(dso)->fd, data, DSO__DATA_CACHE_SIZE, offset);

out:

	pthread_mutex_unlock(&dso__data_open_lock);

	mutex_unlock(dso__data_open_lock());

	return ret;

}
@@ -1078,7 +1099,7 @@ static int file_size(struct dso *dso, struct machine *machine) 
	struct stat st;

	char sbuf[STRERR_BUFSIZE];



	pthread_mutex_lock(&dso__data_open_lock);

	mutex_lock(dso__data_open_lock());



	/*

	 * dso__data(dso)->fd might be closed if other thread opened another
@@ -1102,7 +1123,7 @@ static int file_size(struct dso *dso, struct machine *machine) 
	dso__data(dso)->file_size = st.st_size;



out:

	pthread_mutex_unlock(&dso__data_open_lock);

	mutex_unlock(dso__data_open_lock());

	return ret;

}
@@ -1611,11 +1632,10 @@ size_t dso__fprintf(struct dso *dso, FILE *fp) 


enum dso_type dso__type(struct dso *dso, struct machine *machine)

{

	int fd;

	int fd = -1;

	enum dso_type type = DSO__TYPE_UNKNOWN;



	fd = dso__data_get_fd(dso, machine);

	if (fd >= 0) {

	if (dso__data_get_fd(dso, machine, &fd)) {

		type = dso__type_fd(fd);

		dso__data_put_fd(dso);

	}

tools/perf/util/dso.h

+9 −6
Original line number Diff line number Diff line
@@ -232,6 +232,8 @@ DECLARE_RC_STRUCT(dso) { 
	char		 name[];

};



extern struct mutex _dso__data_open_lock;



/* dso__for_each_symbol - iterate over the symbols of given type

 *

 * @dso: the 'struct dso *' in which symbols are iterated
@@ -653,7 +655,7 @@ void __dso__inject_id(struct dso *dso, const struct dso_id *id); 
int dso__name_len(const struct dso *dso);



struct dso *dso__get(struct dso *dso);

void dso__put(struct dso *dso);

void dso__put(struct dso *dso) LOCKS_EXCLUDED(_dso__data_open_lock);



static inline void __dso__zput(struct dso **dso)

{
@@ -733,8 +735,8 @@ void dso__set_module_info(struct dso *dso, struct kmod_path *m, 
 * The current usage of the dso__data_* interface is as follows:

 *

 * Get DSO's fd:

 *   int fd = dso__data_get_fd(dso, machine);

 *   if (fd >= 0) {

 *   int fd;

 *   if (dso__data_get_fd(dso, machine, &fd)) {

 *       USE 'fd' SOMEHOW

 *       dso__data_put_fd(dso);

 *   }
@@ -756,9 +758,10 @@ void dso__set_module_info(struct dso *dso, struct kmod_path *m, 
 *

 * TODO

*/

int dso__data_get_fd(struct dso *dso, struct machine *machine);

void dso__data_put_fd(struct dso *dso);

void dso__data_close(struct dso *dso);

bool dso__data_get_fd(struct dso *dso, struct machine *machine, int *fd)

	EXCLUSIVE_TRYLOCK_FUNCTION(true, _dso__data_open_lock);

void dso__data_put_fd(struct dso *dso) UNLOCK_FUNCTION(_dso__data_open_lock);

void dso__data_close(struct dso *dso) LOCKS_EXCLUDED(_dso__data_open_lock);



int dso__data_file_size(struct dso *dso, struct machine *machine);

off_t dso__data_size(struct dso *dso, struct machine *machine);

tools/perf/util/unwind-libunwind-local.c

+8 −8
Original line number Diff line number Diff line
@@ -330,8 +330,7 @@ static int read_unwind_spec_eh_frame(struct dso *dso, struct unwind_info *ui, 
	int ret, fd;



	if (dso__data(dso)->eh_frame_hdr_offset == 0) {

		fd = dso__data_get_fd(dso, ui->machine);

		if (fd < 0)

		if (!dso__data_get_fd(dso, ui->machine, &fd))

			return -EINVAL;



		/* Check the .eh_frame section for unwinding info */
@@ -372,8 +371,7 @@ static int read_unwind_spec_debug_frame(struct dso *dso, 
	 *    has to be pointed by symsrc_filename

	 */

	if (ofs == 0) {

		fd = dso__data_get_fd(dso, machine);

		if (fd >= 0) {

		if (dso__data_get_fd(dso, machine, &fd) {

			ofs = elf_section_offset(fd, ".debug_frame");

			dso__data_put_fd(dso);

		}
@@ -485,14 +483,16 @@ find_proc_info(unw_addr_space_t as, unw_word_t ip, unw_proc_info_t *pi, 
	/* Check the .debug_frame section for unwinding info */

	if (ret < 0 &&

	    !read_unwind_spec_debug_frame(dso, ui->machine, &segbase)) {

		int fd = dso__data_get_fd(dso, ui->machine);

		int is_exec = elf_is_exec(fd, dso__name(dso));

		int fd;

		u64 start = map__start(map);

		unw_word_t base = is_exec ? 0 : start;

		unw_word_t base = start;

		const char *symfile;



		if (fd >= 0)

		if (dso__data_get_fd(dso, ui->machine, &fd)) {

			if (elf_is_exec(fd, dso__name(dso)))

				base = 0;

			dso__data_put_fd(dso);

		}



		symfile = dso__symsrc_filename(dso) ?: dso__name(dso);