@jarkko how likely is it that a change in the TPM subsystem leads to cracks in the headphones? 😆

I'm currrently struggling to belive the bisection results, even though they were double-checked 😄

@gromit i might actually might have hunch for this but have been unmotivated to move forward before these patches are merged. One thing at a time IMHO, especially with performance fixes.
NOTE: this is a hyphothesis.

tpm2_get_random() needs to be reconsidered with encryption as hwrng calls it often enough and there's now more overhead.

It pulls entropy in small chunks subtracting the length in the look like you would use read() syscall from user space. This is not right. This leads to small and variable size exchanges and yeah generally that equates to an inefficient implemenation with bad latency properties.

A better implementation would build on two concepts:

1. Have a pool "struct tpm2_random_pool" per caller with a fixed-size buffer in page granularity. E.g. hwrng end of TPM driver would have an instance. This would be exta argument for tpm2_get_random().
2. Have low and high watermarks (or thresholds) for triggering the pull from the chip so that it does not start to throttle.

Normally when the call is made entropy is served from the pool. When going below low water mark a thread (e.g. kthread, workqueue) is woken and less entropy is returned to the caller. Thread then fills the pool with fixes-size requests up until it goes above the high watermark.

I've posted my first patch set early on when the bug was recognized and also reporter confirmed that my fixes do in fact sort out the issue. There's been already some push to also sort some IMA issue but it is chaos if you start to include tons of random fixes to a single patch only because nobody pays any attention.

So I will work on that random pool patch after and also promise not to do anything before my first patch set has landed. With those ideas anyone who wants send me e.g. an RFC patch that I'm happy to give feedback.

@gromit Thanks for asking. I randomly noticed this with custom eBPF traces while debugging the boot-time issue. It is just a guess but this does add more and longer wait states here and there so theoretically could be related but obviously I cannot promise that.

@jarkko

> tpm2_get_random() needs to be reconsidered with encryption as hwrng calls it often enough and there's now more overhead.

Yeah this would indeed make sense, as we landed on "tpm: add session encryption protection to tpm2_get_random()" as a bad commit ...

Is there already a report? If not I will send mine later 🤔

@gromit i don't know but i don't mind getting a report :-) this was great discussion BTW. i had made a theory without experience any "visible issue" just looking at traces. So it is like already in the half-way... thank you!

@gromit I’m happy to do the patch for tpm2_get_random(). It’s kind of fun task anyhow and I think my approach is legit (and tried and tested given this is how memory reclaimers in simplest possible terms).

I was already blown away of the power of eBPF when I found this randomly while fixing another bug. It’s pretty hard to predict performance issues, and now I’m even more blown up and amazed because I had a half-baked solution ready for the oven before there even was a problem ;-)

#bpf #ebpf