On 11/28/22 18:04, Jason A. Donenfeld wrote:
> 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@xxxxxxxxxx> 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@xxxxxxxxxxxx>
>> > >
>> > > 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.
For the record, the bug report is here:
https://lore.kernel.org/all/c5ba47ef-393f-1fba-30bd-1230d1b4b592@xxxxxxx/
(patch author Jan and TPM maintainers were Cc'd)
FWIW I tested the patch 1/3 on top of rc7 and didn't reproduce the issue.
But also had no luck of reproducing it on plain rc7 yet, so it doesn't say
much, unfortunately.
> Thanks,
> Jason
