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 <mail@xxxxxxxxxxxxxxxxxxxxx> >> --- >> 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