On 11/2/24 11:14 AM, Herbert Xu wrote:
On Thu, Oct 24, 2024 at 06:30:15PM +0200, Marek Vasut wrote:
@@ -582,15 +585,12 @@ void hwrng_unregister(struct hwrng *rng)
}
new_rng = get_current_rng_nolock();
- if (list_empty(&rng_list)) {
- mutex_unlock(&rng_mutex);
- if (hwrng_fill)
- kthread_stop(hwrng_fill);
- } else
- mutex_unlock(&rng_mutex);
+ mutex_unlock(&rng_mutex);
if (new_rng)
put_rng(new_rng);
+ else
+ kthread_park(hwrng_fill);
The kthread_park should be moved back into the locked region
of rng_mute). The kthread_stop was moved out because it could
dead-lock waiting on the kthread that's also taking the same
lock. This is no longer an issue with kthread_park since it
simply sets a flag.
Having it outside of the locked region is potentially dangerous
since a pair of hwrng_unregister and hwrng_register could be
re-ordered resulting in the kthread being parked forever.
Sorry for the late reply.
I'm afraid this problem is still present, since kthread_park()
synchronously waits for the kthread to call kthread_parkme(), see
kernel/kthread.c :
655 int kthread_park(struct task_struct *k)
656 {
...
665 set_bit(KTHREAD_SHOULD_PARK, &kthread->flags);
666 if (k != current) {
667 wake_up_process(k);
668 /*
669 * Wait for __kthread_parkme() to complete(), this
means we
670 * _will_ have TASK_PARKED and are about to call
schedule().
671 */
672 wait_for_completion(&kthread->parked);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This part .
The hwrng_fillfn() may call put_rng() which locks rng_mutex() for a
short time, and if kthread_park() is called before hwrng_fillfn() calls
put_rng() within a section protected by rng_mutex too, put_rng() could
never claim rng_mutex and hwrng_fillfn() can never reach
kthread_parkme() call, causing a deadlock.
