On Sun, Jun 22, 2025 at 09:52:58PM +0100, Jonathan McDowell wrote:
> On Fri, Jun 20, 2025 at 06:08:31PM +0000, Orlov, Ivan wrote:
> > The current implementation of timeout detection works in the following
> > way:
> >
> > 1. Read completion status. If completed, return the data
> > 2. Sleep for some time (usleep_range)
> > 3. Check for timeout using current jiffies value. Return an error if
> > timed out
> > 4. Goto 1
> >
> > usleep_range doesn't guarantee it's always going to wake up strictly in
> > (min, max) range, so such a situation is possible:
> >
> > 1. Driver reads completion status. No completion yet
> > 2. Process sleeps indefinitely. In the meantime, TPM responds
> > 3. We check for timeout without checking for the completion again.
> > Result is lost.
> >
> > Such a situation also happens for the guest VMs: if vCPU goes to sleep
> > and doesn't get scheduled for some time, the guest TPM driver will
> > timeout instantly after waking up without checking for the completion
> > (which may already be in place).
> >
> > Perform the completion check once again after exiting the busy loop in
> > order to give the device the last chance to send us some data.
> >
> > Since now we check for completion in two places, extract this check into
> > a separate function.
> >
> > Signed-off-by: Ivan Orlov <ior...@amazon.com>
> > ---
> > V1 -> V2:
> > - Exclude the jiffies -> ktime change from the patch
> > - Instead of recording the time before checking for completion, check
> > for completion once again after leaving the loop
> >
> > drivers/char/tpm/tpm-interface.c | 17 +++++++++++++++--
> > 1 file changed, 15 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm-interface.c
> > b/drivers/char/tpm/tpm-interface.c
> > index 8d7e4da6ed53..6960ee2798e1 100644
> > --- a/drivers/char/tpm/tpm-interface.c
> > +++ b/drivers/char/tpm/tpm-interface.c
> > @@ -82,6 +82,13 @@ static bool tpm_chip_req_canceled(struct tpm_chip *chip,
> > u8 status)
> > return chip->ops->req_canceled(chip, status);
> > }
> >
> > +static bool tpm_transmit_completed(struct tpm_chip *chip)
> > +{
> > + u8 status_masked = tpm_chip_status(chip) & chip->ops->req_complete_mask;
> > +
> > + return status_masked == chip->ops->req_complete_val;
> > +}
> > +
> > static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t
> > bufsiz)
> > {
> > struct tpm_header *header = buf;
> > @@ -129,8 +136,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
> > void *buf, size_t bufsiz)
> > stop = jiffies + tpm_calc_ordinal_duration(chip, ordinal);
> > do {
> > u8 status = tpm_chip_status(chip);
> > - if ((status & chip->ops->req_complete_mask) ==
> > - chip->ops->req_complete_val)
> > + if (tpm_transmit_completed(chip))
> > goto out_recv;
>
> The only thing I'd point out here is we end up doing a double status read
> one after the other (once here, once in tpm_transmit_completed), and I'm
> pretty sure I've seen instances where that caused a problem.
It would be easy to to prevent at least double reads after completion
e.g., in tpm_chip_status():
/*
* Read the chip status bitmask. After completion, the returned will mask will
* return value cached at the point of completion up until the next transmit.
*/
static u8 tpm_chip_status(struct tpm_chip *chip)
{
u8 status_masked = chip->status & chip->ops_req_complete_mask;
if (status_masked == chip->ops->req_complete_val)
return chip->status;
chip->status = tpm_chip_status(chip);
return chip->status;
}
I think tpm_chip_status() should be the gatekeeper for such event, or
like the correct layer of abstraction here ...
Then just reset chip->status to zero at the beginning of tpm_try_transmit().
BR, Jarkko
