Re: [PATCH v2 1/3] char: tpm: Protect tpm_pm_suspend with locks

["Jason A. Donenfeld" <Jason@zx2c4.com>]

Hi Jarkko,

On Mon, Nov 07, 2022 at 06:35:25PM +0200, Jarkko Sakkinen wrote:
> On Mon, Nov 07, 2022 at 09:45:41AM +0100, Jan Dąbroś wrote:
> > niedz., 6 lis 2022 o 20:49 Jarkko Sakkinen <jarkko@kernel.org> napisał(a):
> > >
> > > On Thu, Nov 03, 2022 at 03:54:48PM +0100, Jan Dabros wrote:
> > > > Currently tpm transactions are executed unconditionally in
> > > > tpm_pm_suspend() function, what may lead to races with other tpm
> > > > accessors in the system.
> > > >
> > > > Add proper locking mechanisms by calling tpm_try_get_ops() which is a
> > > > wrapper on tpm_chip_start().
> > > >
> > > > Signed-off-by: Jan Dabros <jsd@semihalf.com>
> > >
> > > AFAIK processes are freezed before suspend callbacks are called, and
> > > the callbacks are called sequentially. I have no idea what is meant
> > > by "TPM accessor" here.
> > 
> > User space processes are freezed before suspend, but kernel threads
> > are not freezable by default. In my particular case it was a hwrng
> > thread started from drivers/char/hw_random/core.c - I was referring to
> > it as "TPM accessor". For sure I should be more precise in a commit
> > msg.
> 
> OK, great.
> 
> > 
> > > Please describe the concurrency scenario in the commit message where the
> > > race could happen, if it is hard to reproduce, and add an appropriate fixes
> > > tag.
> > 
> > I will describe my scenario in more detail in the next version.
> > Regarding the "fixes" tag - I'm not too familiar with it, but looking
> > at the kernel submission guide, "fixes" should be used either when
> > there was a particular commit in the past which introduced the bug or
> > if a patch fixes an already logged bug entry (so that one can paste
> > URL). In my case both are not applicable, so please advise what
> > exactly I should put after this tag?
> 
> It is 
> 
> Fixed: <12 character prefix of the hash> ("short summary")
> 
> It should point out to the commit, which introduced the issue/bug.

Somebody just reported a bug to me in hwrng stuff, which I traced down
to this lack of tpm_try_get_ops(), and then I coded up the exact same
patch as Jan's here, saw it fixed the problem, and then realized it was
already submitted in that exact form.

Because of some recent hwrng changes, this bug will be triggered more
often than it was before.

So, even though Jan hasn't submitted a v+1, do you think you could take
this 1/3 commit, fix up the commit message or whatever you need there,
and get this in for 6.1-rc8? It's an important fix.

Thanks,
Jason