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

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

[hyunjungg]

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>
---
v2: Rename the local bool flush_timeout -> flush, per Jarkko's review.
    No functional change.
 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..97c1b7c 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 = false;
 	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 = 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)
+		flush_work(&priv->timeout_work);
+
 	return ret_size;
 }
 
-- 
2.43.0

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

[Jarkko Sakkinen]

On Sun, Nov 08, 2026 at 09:32:46PM +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.

I 100% understand the bug fix but why we need to write an article
to describe a trivial bug? It's long enough that I lost the plot
before finishing it up. Have you *ever* tried to read it yourself?

> 
> 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.

I think the last paragraph should be deleted.

> 
> Fixes: 9488585b21be ("tpm: add support for partial reads")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hyunjung Ko <hj351016@gmail.com>
> ---
> v2: Rename the local bool flush_timeout -> flush, per Jarkko's review.
>     No functional change.
>  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..97c1b7c 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 = false;
>  	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 = 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.
> +	 */

I missed this one but I don't think we need this remark for a trivial
bug fix. It's 50% of the whole patch. Please, delete it.


> +	if (flush)
> +		flush_work(&priv->timeout_work);
> +
>  	return ret_size;
>  }
>  
> -- 
> 2.43.0
> 

The comment had the pretty much had the relevant information. Please
next time don't submit RAW LLM output and make sure that you are able
to at least the commit message. This is horrible underperforming in 
my books.

Br, Jarkko