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

[khj]

Thanks, renamed to 'flush' in v2.

2026년 11월 8일 (일) 오후 9:29, Jarkko Sakkinen <jarkko@xxxxxxxxxx>님이 작성:
>
> On Sun, Nov 08, 2026 at 04:58:48PM +0900, hyunjungg wrote:
> > From: Hyunjung Ko <hj351016@xxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx
> > Signed-off-by: Hyunjung Ko <hj351016@xxxxxxxxx>
> > ---
> >  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