Re: [PATCH v2 16/19] perf mutex: Add a robust, process-shared mutex initializer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf/mutex: mutex_trylock and cond_wait break robust mutexes
--

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

perf mutex: Add a robust, process-shared mutex initializer

A mutex held by a process that can die before unlocking it, like the
tests forked by 'perf test' that will take turns using the per user
budget set in kernel.perf_event_mlock_kb, has to be taken over by the
next locker instead of staying locked forever.

Robust mutexes, on top of the kernel's robust futexes, do that: when an
owner dies while holding it the kernel marks the futex word owner-died
and the next pthread_mutex_lock() returns EOWNERDEAD, then mutex_lock()
makes it consistent and takes it over.

> diff --git a/tools/perf/util/mutex.c b/tools/perf/util/mutex.c
> index 7aa1f3f55a7d4..712fbf839d590 100644
> --- a/tools/perf/util/mutex.c
> +++ b/tools/perf/util/mutex.c

[ ... ]

> @@ -58,7 +105,22 @@ void mutex_destroy(struct mutex *mtx)
>  void mutex_lock(struct mutex *mtx)
>  	NO_THREAD_SAFETY_ANALYSIS
>  {
> +#ifdef HAVE_PTHREAD_MUTEX_ROBUST
> +	int err = pthread_mutex_lock(&mtx->lock);
> +
> +	/*
> +	 * A robust mutex whose owner died while holding it comes back as
> +	 * EOWNERDEAD, marked by the kernel: take it over.
> +	 */
> +	if (err == EOWNERDEAD) {
> +		pr_debug("mutex: taking over one left by a dead owner\n");
> +		CHECK_ERR(pthread_mutex_consistent(&mtx->lock));
> +	} else {
> +		CHECK_ERR(err);
> +	}
> +#else
>  	CHECK_ERR(pthread_mutex_lock(&mtx->lock));
> +#endif
>  }

[Severity: Medium]
Since this patch introduces robust mutexes and adds EOWNERDEAD handling to
mutex_lock(), does it leave mutex_trylock() and cond_wait() broken when used
with them?

If pthread_mutex_trylock() returns EOWNERDEAD, the lock is acquired by the
caller, but mutex_trylock() currently treats it as an error:

tools/perf/util/mutex.c:mutex_trylock() {
        ...
        if (ret == 0)
                return true; /* Lock acquired. */

        if (ret == EBUSY)
                return false; /* Lock busy. */

        /* Print error. */
        CHECK_ERR(ret);
        return false;
}

This will silently leak the lock and cause the caller to incorrectly assume
the lock is not held.

Similarly, if pthread_cond_wait() returns EOWNERDEAD:

tools/perf/util/mutex.c:cond_wait() {
        CHECK_ERR(pthread_cond_wait(&cnd->cond, &mtx->lock));
}

The mutex is re-acquired but inconsistent. Failing to call
pthread_mutex_consistent() before the caller unlocks it will permanently
break the mutex.

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