Re: git: 87ee63bac69d - main - locks: add a runtime check for missing turnstile

On 7/11/24 07:07, Mateusz Guzik wrote:
> The branch main has been updated by mjg:
> 
> URL: https://cgit.FreeBSD.org/src/commit/?id=87ee63bac69dc49291f55590b8baa57cad6c7d85
> 
> commit 87ee63bac69dc49291f55590b8baa57cad6c7d85
> Author:     Mateusz Guzik <mjg@FreeBSD.org>
> AuthorDate: 2024-07-11 00:17:27 +0000
> Commit:     Mateusz Guzik <mjg@FreeBSD.org>
> CommitDate: 2024-07-11 11:06:52 +0000
> 
>      locks: add a runtime check for missing turnstile
>      
>      There are sometimes bugs which result in the unlock fast path failing,
>      which in turns causes a not-helpful crash report when dereferencing a
>      NULL turnstile. Help debugging such cases by pointing out what happened
>      along with some debug.
>      
>      Sponsored by:   Rubicon Communications, LLC ("Netgate")
> ---
>   sys/kern/kern_mutex.c  |  4 +++-
>   sys/kern/kern_rwlock.c | 16 ++++++++++++----
>   2 files changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/sys/kern/kern_mutex.c b/sys/kern/kern_mutex.c
> index 90361b23c09a..0fa624cc4bb1 100644
> --- a/sys/kern/kern_mutex.c
> +++ b/sys/kern/kern_mutex.c
> @@ -1053,7 +1053,9 @@ __mtx_unlock_sleep(volatile uintptr_t *c, uintptr_t v)
>   	turnstile_chain_lock(&m->lock_object);
>   	_mtx_release_lock_quick(m);
>   	ts = turnstile_lookup(&m->lock_object);
> -	MPASS(ts != NULL);
> +	if (__predict_false(ts == NULL)) {
> +		panic("got NULL turnstile on mutex %p v %zx", m, v);
> +	}

Hmm, this is just an expanded KASSERT() but always on rather than conditional on INVARIANTS?

Do you have examples of the type of bugs that cause this?  (Is it unlocking a freed mutex
or the like?)  We generally hide all these types of checks under INVARIANTS rather than
shipping them in release kernels.

-- 
John Baldwin