| Message ID | 155326553839.17270.7997105682830234546.stgit@tstruk-mobl1.jf.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND,v3] tpm: fix an invalid condition in tpm_common_poll | expand |
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: stable@vger.kernel.org > Fixes: 9488585b21bef0df12 ("tpm: add support for partial reads") > Reported-by: Mantas Mikulėnas <grawity@gmail.com> > Tested-by: Mantas Mikulėnas <grawity@gmail.com> > Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> 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: stable@vger.kernel.org > > Fixes: 9488585b21bef0df12 ("tpm: add support for partial reads") > > Reported-by: Mantas Mikulėnas <grawity@gmail.com> > > Tested-by: Mantas Mikulėnas <grawity@gmail.com> > > Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com> > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> 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,
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
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; }
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: stable@vger.kernel.org Fixes: 9488585b21bef0df12 ("tpm: add support for partial reads") Reported-by: Mantas Mikulėnas <grawity@gmail.com> Tested-by: Mantas Mikulėnas <grawity@gmail.com> Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com> --- drivers/char/tpm/tpm-dev-common.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)