Re: [PATCH] tpm: flush timeout work outside buffer_mutex to avoid deadlock

[Jarkko Sakkinen <jarkko@kernel.org>]

On Sun, Nov 08, 2026 at 04:58:48PM +0900, hyunjungg wrote:
> From: Hyunjung Ko <hj351016@gmail.com>
> 
> tpm_common_read() flushes priv->timeout_work while holding
> priv->buffer_mutex:
> 
> 	mutex_lock(&priv->buffer_mutex);
> 	...
> 	if (!priv->response_length) {
> 		*off = 0;
> 		timer_delete_sync(&priv->user_read_timer);
> 		flush_work(&priv->timeout_work);
> 	}
> 	mutex_unlock(&priv->buffer_mutex);
> 
> The flushed handler, tpm_timeout_work(), takes the same buffer_mutex at
> its start. user_reader_timeout() schedules timeout_work from the
> user_read_timer, which is armed while a response is pending. If that
> timer fires and timeout_work starts running while a reader holds
> buffer_mutex, flush_work() waits for timeout_work to finish while the
> reader holds the mutex, and timeout_work blocks acquiring that same
> mutex -> deadlock. lockdep would report this as a circular locking
> dependency between buffer_mutex and the work.
> 
> Before commit 9488585b21be ("tpm: add support for partial reads") the
> timer deletion and the work flush ran before buffer_mutex was taken;
> that commit moved them into the mutex-held section, introducing the
> flush-under-lock cycle.
> 
> Flush timeout_work after dropping buffer_mutex. The timer is still
> deleted under the lock, so no new timeout work can be queued once the
> mutex is released, and the flush no longer runs under the lock the work
> needs.
> 
> Found by static analysis (a checker for cancel/flush-sync of a work
> performed under a lock that the work's handler also takes) and confirmed
> by code inspection. Not reproduced at runtime: it requires the
> deprecated user-space read timeout to fire while a read holds
> buffer_mutex, so the deadlock is established by inspection rather than an
> observed lockdep splat.
> 
> Fixes: 9488585b21be ("tpm: add support for partial reads")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hyunjung Ko <hj351016@gmail.com>
> ---
>  drivers/char/tpm/tpm-dev-common.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
> index 6569212..f9b18ef 100644
> --- a/drivers/char/tpm/tpm-dev-common.c
> +++ b/drivers/char/tpm/tpm-dev-common.c
> @@ -132,6 +132,7 @@ ssize_t tpm_common_read(struct file *file, char __user *buf,
>  {
>  	struct file_priv *priv = file->private_data;
>  	ssize_t ret_size = 0;
> +	bool flush_timeout = false;

Rename this as flush.

>  	int rc;
>  
>  	if (!size)
> @@ -164,9 +165,19 @@ out:
>  	if (!priv->response_length) {
>  		*off = 0;
>  		timer_delete_sync(&priv->user_read_timer);
> -		flush_work(&priv->timeout_work);
> +		flush_timeout = true;
>  	}
>  	mutex_unlock(&priv->buffer_mutex);
> +
> +	/*
> +	 * tpm_timeout_work() acquires buffer_mutex, so flushing it while that
> +	 * mutex is held deadlocks if the timeout work is already running and
> +	 * waiting for the lock. Flush after dropping the mutex; the timer is
> +	 * deleted above under the lock, so no new timeout work can be queued.
> +	 */
> +	if (flush_timeout)
> +		flush_work(&priv->timeout_work);
> +
>  	return ret_size;
>  }
>  
> -- 
> 2.43.0
> 

Br, Jarkko