Since commit 59b569480dc8
("random: Use wait_event_freezable() in add_hwgenerator_randomness()")
there is a race in add_hwgenerator_randomness() between freezing and
stopping the calling kthread.
This commit changed wait_event_interruptible() call with
kthread_freezable_should_stop() as a condition into wait_event_freezable()
with just kthread_should_stop() as a condition to fix a warning that
kthread_freezable_should_stop() might sleep inside the wait.
wait_event_freezable() ultimately calls __refrigerator() with its
check_kthr_stop argument set to false, which causes it to keep the kthread
frozen even if somebody calls kthread_stop() on it.
Calling wait_event_freezable() with kthread_should_stop() as a condition
is racy because it doesn't take into account the situation where this
condition becomes true on a kthread marked for freezing only after this
condition has already been checked.
Calling freezing() should avoid the issue that the commit 59b569480dc8 has
fixed, as it is only a checking function, it doesn't actually do the
freezing.
add_hwgenerator_randomness() has two post-boot users: in khwrng the
kthread will be frozen anyway by call to kthread_freezable_should_stop()
in its main loop, while its second user (ath9k-hwrng) is not freezable at
all.
This change allows a VM with virtio-rng loaded to write s2disk image
successfully.
Fixes: 59b569480dc8 ("random: Use wait_event_freezable() in add_hwgenerator_randomness()")
Signed-off-by: Maciej S. Szmigiero <[email protected]>
---
drivers/char/random.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/char/random.c b/drivers/char/random.c
index de434feb873a..2f87910dd498 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -2500,8 +2500,8 @@ void add_hwgenerator_randomness(const char *buffer, size_t count,
* We'll be woken up again once below random_write_wakeup_thresh,
* or when the calling thread is about to terminate.
*/
- wait_event_freezable(random_write_wait,
- kthread_should_stop() ||
+ wait_event_interruptible(random_write_wait,
+ kthread_should_stop() || freezing(current) ||
ENTROPY_BITS(&input_pool) <= random_write_wakeup_bits);
mix_pool_bytes(poolp, buffer, count);
credit_entropy_bits(poolp, entropy);
On Sun, Nov 10, 2019 at 02:55:42PM +0100, Maciej S. Szmigiero wrote:
> Since commit 59b569480dc8
> ("random: Use wait_event_freezable() in add_hwgenerator_randomness()")
> there is a race in add_hwgenerator_randomness() between freezing and
> stopping the calling kthread.
>
> This commit changed wait_event_interruptible() call with
> kthread_freezable_should_stop() as a condition into wait_event_freezable()
> with just kthread_should_stop() as a condition to fix a warning that
> kthread_freezable_should_stop() might sleep inside the wait.
>
> wait_event_freezable() ultimately calls __refrigerator() with its
> check_kthr_stop argument set to false, which causes it to keep the kthread
> frozen even if somebody calls kthread_stop() on it.
>
> Calling wait_event_freezable() with kthread_should_stop() as a condition
> is racy because it doesn't take into account the situation where this
> condition becomes true on a kthread marked for freezing only after this
> condition has already been checked.
>
> Calling freezing() should avoid the issue that the commit 59b569480dc8 has
> fixed, as it is only a checking function, it doesn't actually do the
> freezing.
>
> add_hwgenerator_randomness() has two post-boot users: in khwrng the
> kthread will be frozen anyway by call to kthread_freezable_should_stop()
> in its main loop, while its second user (ath9k-hwrng) is not freezable at
> all.
>
> This change allows a VM with virtio-rng loaded to write s2disk image
> successfully.
>
> Fixes: 59b569480dc8 ("random: Use wait_event_freezable() in add_hwgenerator_randomness()")
> Signed-off-by: Maciej S. Szmigiero <[email protected]>
> ---
> drivers/char/random.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
Patch applied. Thanks.
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On 15.11.2019 18:44, Stephen Boyd wrote:
> Quoting Maciej S. Szmigiero (2019-11-10 05:55:42)
>> Since commit 59b569480dc8
>> ("random: Use wait_event_freezable() in add_hwgenerator_randomness()")
>> there is a race in add_hwgenerator_randomness() between freezing and
>> stopping the calling kthread.
>>
>> This commit changed wait_event_interruptible() call with
>> kthread_freezable_should_stop() as a condition into wait_event_freezable()
>> with just kthread_should_stop() as a condition to fix a warning that
>> kthread_freezable_should_stop() might sleep inside the wait.
>>
>> wait_event_freezable() ultimately calls __refrigerator() with its
>> check_kthr_stop argument set to false, which causes it to keep the kthread
>> frozen even if somebody calls kthread_stop() on it.
>>
>> Calling wait_event_freezable() with kthread_should_stop() as a condition
>> is racy because it doesn't take into account the situation where this
>> condition becomes true on a kthread marked for freezing only after this
>> condition has already been checked.
>>
>> Calling freezing() should avoid the issue that the commit 59b569480dc8 has
>> fixed, as it is only a checking function, it doesn't actually do the
>> freezing.
>>
>> add_hwgenerator_randomness() has two post-boot users: in khwrng the
>> kthread will be frozen anyway by call to kthread_freezable_should_stop()
>> in its main loop, while its second user (ath9k-hwrng) is not freezable at
>> all.
>>
>> This change allows a VM with virtio-rng loaded to write s2disk image
>> successfully.
>>
>> Fixes: 59b569480dc8 ("random: Use wait_event_freezable() in add_hwgenerator_randomness()")
>> Signed-off-by: Maciej S. Szmigiero <[email protected]>
>> ---
>> drivers/char/random.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/char/random.c b/drivers/char/random.c
>> index de434feb873a..2f87910dd498 100644
>> --- a/drivers/char/random.c
>> +++ b/drivers/char/random.c
>> @@ -2500,8 +2500,8 @@ void add_hwgenerator_randomness(const char *buffer, size_t count,
>> * We'll be woken up again once below random_write_wakeup_thresh,
>> * or when the calling thread is about to terminate.
>> */
>> - wait_event_freezable(random_write_wait,
>> - kthread_should_stop() ||
>> + wait_event_interruptible(random_write_wait,
>> + kthread_should_stop() || freezing(current) ||
>> ENTROPY_BITS(&input_pool) <= random_write_wakeup_bits);
>
> Is it a problem that this wakes up, sees that it should freeze but then
> calls mix_pool_bytes() and credit_entropy_bits()? It looks like
> credit_entropy_bits() will try to wakeup a reader task that is already
> frozen (see the wake_up_interruptible(&random_read_wait) call).
If a reader (user space) task is frozen then it is no longer waiting
on this waitqueue - at least if I understand correctly how the freezer
works for user space tasks, that is by interrupting waits via a fake
signal.
> At one> point I was checking to see if the task was freezing and avoided calling
> those functions so we could get back to kthread_freezable_should_stop()
> in the kthread and actually freeze.
Yes, but I think mixing some extra valid data into random buffer in this
very rare situation shouldn't hurt.
>> mix_pool_bytes(poolp, buffer, count);
>> credit_entropy_bits(poolp, entropy);
>
> It's almost like we need a wait_event_freezable_stoppable() API that
> will freeze if freezing() and break out if the kthread is stopped and
> otherwise wait for a wakeup to test the condition. Basically the same
> sort of API that we have for wait_event_freezable() but we pass true for
> the check_kthr_stop flag. Then we can have something like this:
>
> wait_event_freezable_stoppable(
> ENTROPY_BITS(&input_pool) <= random_write_wakeup_bits);
> if (!kthread_should_stop()) {
> mix_pool_bytes(...);
> credit_entropy_bits(...);
> }
>
This API could be added but will there be other users for it?
I mean I think it might not be worth adding a new core kernel API for
such a specific, and workaroundable, issue.
But, on the other hand, maybe this would be a cleaner solution, even
though it would be more complicated.
By the way, the same goes for something like set_freezable_only()
function for the second khwrng freezing issue that I have mentioned
in my Nov 10 UTC message.
Maciej
On Sun, Nov 17, 2019 at 12:01:20AM +0100, Maciej S. Szmigiero wrote:
>
> If a reader (user space) task is frozen then it is no longer waiting
> on this waitqueue - at least if I understand correctly how the freezer
> works for user space tasks, that is by interrupting waits via a fake
> signal.
At this point I'm just going to revert the whole thing and we can
sort it out in the next development cycle.
Thanks,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Sun, Nov 17, 2019 at 08:56:41AM +0800, Herbert Xu wrote:
> On Sun, Nov 17, 2019 at 12:01:20AM +0100, Maciej S. Szmigiero wrote:
> >
> > If a reader (user space) task is frozen then it is no longer waiting
> > on this waitqueue - at least if I understand correctly how the freezer
> > works for user space tasks, that is by interrupting waits via a fake
> > signal.
>
> At this point I'm just going to revert the whole thing and we can
> sort it out in the next development cycle.
Thanks, I hadn't planned on taking any /dev/random changes this cycle
because it had gotten too late and the ext4 tree had gotten unusually
busy.
- Ted
On 11/17/2019 2:57 AM, Herbert Xu wrote:
> On Sun, Nov 17, 2019 at 12:01:20AM +0100, Maciej S. Szmigiero wrote:
>>
>> If a reader (user space) task is frozen then it is no longer waiting
>> on this waitqueue - at least if I understand correctly how the freezer
>> works for user space tasks, that is by interrupting waits via a fake
>> signal.
>
> At this point I'm just going to revert the whole thing and we can
> sort it out in the next development cycle.
>
Working at adding power management support for caam crypto driver,
I've stumbled upon the issue of caam rng suspending while hwrng kthread
is still active.
For now I've fixed the issue by unregistering / re-registering caam rng
from hwrng at device suspend / resume time.
OTOH I see a few commits:
03a3bb7ae631 ("hwrng: core - Freeze khwrng thread during suspend")
ff296293b353 ("random: Support freezable kthreads in add_hwgenerator_randomness()")
59b569480dc8 ("random: Use wait_event_freezable() in add_hwgenerator_randomness()")
however they were reverted a bit later in commit
08e97aec700a ("Revert "hwrng: core - Freeze khwrng thread during suspend"")
I wasn't able to find a follow-up on this topic.
Could you advise on what's the proper thing to do going forward?
Thanks,
Horia
On Wed, Dec 02, 2020 at 04:28:07PM +0200, Horia Geantă wrote:
>
> I wasn't able to find a follow-up on this topic.
> Could you advise on what's the proper thing to do going forward?
I think someone needs to come up with a patch that does not
cause regressions.
Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt