Re: [PATCH v2] tpm: Fix the timeout & use ktime

[Jonathan McDowell <noodles@earth.li>]

On Fri, Jul 04, 2025 at 04:39:20PM +0100, Orlov, Ivan wrote:
>On 04/07/2025 16:16, Jarkko Sakkinen wrote:
>>On Fri, Jul 04, 2025 at 10:02:33AM +0100, Jonathan McDowell wrote:
>>>On Wed, Jun 25, 2025 at 07:43:07PM +0300, Jarkko Sakkinen wrote:
>>>>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 <iorlov@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():
>>>
>>>Or just take the simple approach and make the check after the while loop:
>>>
>>>	if ((tpm_chip_status(chip) & chip->ops->req_complete_mask) ==
>>>	    chip->ops->req_complete_val)
>>>		goto out_recv;
>>>
>>>There might be potential for a longer term cleanup using chip->status to
>>>cache things, but I'm little concerned that's going to open paths where we
>>>might not correctly update it, so I think it should be a separate piece.
>>>
>>>(I'm motivated by the fact we've started to see the "Operation Canceled"
>>>error and I'd like us to close on the best way to fix it. :) )
>>
>>This would work for me too!
>>
>
>Hi, and sorry for the late reply :(
>
>I believe this option would work for us as well. Please let me know 
>whether you'd like me to send V3 or you feel free to send it yourself 
>if you want.

As far as I'm concerned it's something you found, and your patch. You 
send a new version, I'll test + review it. :)

J.

-- 
  What have you got in your pocket? |  .''`.  Debian GNU/Linux Developer
                                    | : :' :  Happy to accept PGP signed
                                    | `. `'   or encrypted mail - RSA
                                    |   `-    key on the keyservers.