On Thu, Oct 31, 2024 at 01:36:46AM +0200, Jarkko Sakkinen wrote: > On Wed Oct 30, 2024 at 10:09 PM EET, Jerry Snitselaar wrote: > > On Wed, Oct 30, 2024 at 12:36:47AM +0200, Jarkko Sakkinen wrote: > > > Setting TPM_CHIP_FLAG_SUSPENDED in the end of tpm_pm_suspend() can be racy > > > according to the bug report, as this leaves window for tpm_hwrng_read() to > > > be called while the operation is in progress. Move setting of the flag > > > into the beginning. > > > > > > Cc: [email protected] # v6.4+ > > > Fixes: 99d464506255 ("tpm: Prevent hwrng from activating during resume") > > > Reported-by: Mike Seo <[email protected]> > > > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219383 > > > Signed-off-by: Jarkko Sakkinen <[email protected]> > > > --- > > > drivers/char/tpm/tpm-interface.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/char/tpm/tpm-interface.c > > > b/drivers/char/tpm/tpm-interface.c > > > index 8134f002b121..3f96bc8b95df 100644 > > > --- a/drivers/char/tpm/tpm-interface.c > > > +++ b/drivers/char/tpm/tpm-interface.c > > > @@ -370,6 +370,8 @@ int tpm_pm_suspend(struct device *dev) > > > if (!chip) > > > return -ENODEV; > > > > > > + chip->flags |= TPM_CHIP_FLAG_SUSPENDED; > > > + > > > if (chip->flags & TPM_CHIP_FLAG_ALWAYS_POWERED) > > > goto suspended; > > > > > > @@ -390,8 +392,6 @@ int tpm_pm_suspend(struct device *dev) > > > } > > > > > > suspended: > > > - chip->flags |= TPM_CHIP_FLAG_SUSPENDED; > > > - > > > if (rc) > > > dev_err(dev, "Ignoring error %d while suspending\n", rc); > > > return 0; > > > -- > > > 2.47.0 > > > > > > > Reviewed-by: Jerry Snitselaar <[email protected]> > > Thanks but I actually started to look at the function: > > https://elixir.bootlin.com/linux/v6.11.5/source/drivers/char/tpm/tpm-interface.c#L365 > > The absolutely safe-play way considering concurrency would be > to do tpm_try_get_ops() before checking any flags. That way > tpm_hwrng_read() is guaranteed not conflict. > > So the way I would fix this instead would be to (untested > wrote inline here): > > int tpm_pm_suspend(struct device *dev) > { > struct tpm_chip *chip = dev_get_drvdata(dev); > int rc = 0; > > if (!chip) > return -ENODEV; > > rc = tpm_try_get_ops(chip); > if (rc) { > chip->flags = |= TPM_CHIP_FLAG_SUSPENDED; > return rc; > } > > /* ... */ > > suspended: > chip->flags |= TPM_CHIP_FLAG_SUSPENDED; > tpm_put_ops(chip); > > It does not really affect performance but guarantees that > tpm_hwrng_read() is guaranteed either fully finish or > never happens given that both sides take chip->lock. > > So I'll put one more round of this and then this should be > stable and fully fixed. > > BR, Jarkko
Ah, yeah better to set it while it has the mutex. That should still be 'if (!rc)' after the tpm_try_get_ops() right? (I'm assuming that is just a transcription error). Regards, Jerry
