The poll condition should only check response_length,
because reads should only be issued if there is data to read.
The response_read flag only prevents double writes.
The problem was that the write set the response_read to false,
enqued a tpm job, and returned. Then application called poll
which checked the response_read flag and returned EPOLLIN.
Then the application called read, but got nothing.
After all that the async_work kicked in.
Added also mutex_lock around the poll check to prevent
other possible race conditions.
Cc: [email protected]
Fixes: 9488585b21bef0df12 ("tpm: add support for partial reads")
Reported-by: Mantas Mikulėnas <[email protected]>
Tested-by: Mantas Mikulėnas <[email protected]>
Signed-off-by: Tadeusz Struk <[email protected]>
---
drivers/char/tpm/tpm-dev-common.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
index 5eecad233ea1..7312d3214381 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -203,12 +203,14 @@ __poll_t tpm_common_poll(struct file *file, poll_table *wait)
__poll_t mask = 0;
poll_wait(file, &priv->async_wait, wait);
+ mutex_lock(&priv->buffer_mutex);
- if (!priv->response_read || priv->response_length)
+ if (priv->response_length)
mask = EPOLLIN | EPOLLRDNORM;
else
mask = EPOLLOUT | EPOLLWRNORM;
+ mutex_unlock(&priv->buffer_mutex);
return mask;
}
On Fri, Mar 22, 2019 at 07:38:58AM -0700, Tadeusz Struk wrote:
> The poll condition should only check response_length,
> because reads should only be issued if there is data to read.
> The response_read flag only prevents double writes.
> The problem was that the write set the response_read to false,
> enqued a tpm job, and returned. Then application called poll
> which checked the response_read flag and returned EPOLLIN.
> Then the application called read, but got nothing.
> After all that the async_work kicked in.
> Added also mutex_lock around the poll check to prevent
> other possible race conditions.
>
> Cc: [email protected]
> Fixes: 9488585b21bef0df12 ("tpm: add support for partial reads")
> Reported-by: Mantas Mikulėnas <[email protected]>
> Tested-by: Mantas Mikulėnas <[email protected]>
> Signed-off-by: Tadeusz Struk <[email protected]>
Reviewed-by: Jarkko Sakkinen <[email protected]>
Thank you.
/Jarkko
On Fri, Mar 22, 2019 at 05:59:17PM +0200, Jarkko Sakkinen wrote:
> On Fri, Mar 22, 2019 at 07:38:58AM -0700, Tadeusz Struk wrote:
> > The poll condition should only check response_length,
> > because reads should only be issued if there is data to read.
> > The response_read flag only prevents double writes.
> > The problem was that the write set the response_read to false,
> > enqued a tpm job, and returned. Then application called poll
> > which checked the response_read flag and returned EPOLLIN.
> > Then the application called read, but got nothing.
> > After all that the async_work kicked in.
> > Added also mutex_lock around the poll check to prevent
> > other possible race conditions.
> >
> > Cc: [email protected]
> > Fixes: 9488585b21bef0df12 ("tpm: add support for partial reads")
> > Reported-by: Mantas Mikulėnas <[email protected]>
> > Tested-by: Mantas Mikulėnas <[email protected]>
> > Signed-off-by: Tadeusz Struk <[email protected]>
>
> Reviewed-by: Jarkko Sakkinen <[email protected]>
It is still missing the comment I asked to add. Otherwise, it is good.
/Jarkko
Hi Jarkko,
On 3/25/19 7:09 AM, Jarkko Sakkinen wrote:
> It is still missing the comment I asked to add. Otherwise, it is good.
>
Sorry, I didn't see your email with the suggestion earlier.
To be honest I'm not sure if this comment adds much value, or if it is
even correct. The poll doesn't "succeed" or "fail". It just returns
a mask indicating if there is any data to read or if the user can write.
Isn't the commit message + 'git blame' enough to remember why it was
done this way?
Thanks,
--
Tadeusz
On Tue, Mar 26, 2019 at 08:58:28AM -0700, Tadeusz Struk wrote:
> Hi Jarkko,
> On 3/25/19 7:09 AM, Jarkko Sakkinen wrote:
> > It is still missing the comment I asked to add. Otherwise, it is good.
> >
>
> Sorry, I didn't see your email with the suggestion earlier.
> To be honest I'm not sure if this comment adds much value, or if it is
> even correct. The poll doesn't "succeed" or "fail". It just returns
> a mask indicating if there is any data to read or if the user can write.
>
> Isn't the commit message + 'git blame' enough to remember why it was
> done this way?
Comments in the code have also their time and place especially when
doing code reviews. Usually I like to have something in a site where
there has been a race even if it was for fairly trivial reason. If you
want to refine the comment to be more to the point, that is perfectly
fine.
/Jarkko