There are two deadlock scenarios that need addressing, which cause problems when the computer goes to sleep, the interface is set down, and hwrng_unregister() is called. When the deadlock is hit, sleep is delayed for tens of seconds, causing it to fail. These scenarios are:
1) The hwrng kthread can't be stopped while it's sleeping, because it uses msleep_interruptible() instead of schedule_timeout_interruptible(). The fix is a simple moving to the correct function. At the same time, we should cleanup a common and useless dmesg splat in the same area.
2) A normal user thread can't be interrupted by hwrng_unregister() while it's sleeping, because hwrng_unregister() is called from elsewhere. The solution here is to keep track of which thread is currently reading, and asleep, and signal that thread when it's time to unregister. There's a bit of book keeping required to prevent lifetime issues on current.
Reported-by: Gregory Erwin gregerwin256@gmail.com Cc: Toke Høiland-Jørgensen toke@redhat.com Cc: Kalle Valo kvalo@kernel.org Cc: Rui Salvaterra rsalvaterra@gmail.com Cc: Herbert Xu herbert@gondor.apana.org.au Cc: stable@vger.kernel.org Fixes: fcd09c90c3c5 ("ath9k: use hw_random API instead of directly dumping into random.c") Link: https://lore.kernel.org/all/CAO+Okf6ZJC5-nTE_EJUGQtd8JiCkiEHytGgDsFGTEjs0c00... Link: https://lore.kernel.org/lkml/CAO+Okf5k+C+SE6pMVfPf-d8MfVPVq4PO7EY8Hys_DVXten... Link: https://bugs.archlinux.org/task/75138 Signed-off-by: Jason A. Donenfeld Jason@zx2c4.com --- drivers/char/hw_random/core.c | 30 ++++++++++++++++++++++++---- drivers/net/wireless/ath/ath9k/rng.c | 19 +++++++----------- 2 files changed, 33 insertions(+), 16 deletions(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c index 16f227b995e8..df45c265878e 100644 --- a/drivers/char/hw_random/core.c +++ b/drivers/char/hw_random/core.c @@ -38,6 +38,8 @@ static LIST_HEAD(rng_list); static DEFINE_MUTEX(rng_mutex); /* Protects rng read functions, data_avail, rng_buffer and rng_fillbuf */ static DEFINE_MUTEX(reading_mutex); +/* Keeps track of whoever is wait-reading it currently while holding reading_mutex. */ +static struct task_struct *current_waiting_reader; static int data_avail; static u8 *rng_buffer, *rng_fillbuf; static unsigned short current_quality; @@ -208,6 +210,7 @@ static ssize_t rng_dev_read(struct file *filp, char __user *buf, int err = 0; int bytes_read, len; struct hwrng *rng; + bool wait;
while (size) { rng = get_current_rng(); @@ -225,9 +228,15 @@ static ssize_t rng_dev_read(struct file *filp, char __user *buf, goto out_put; } if (!data_avail) { + wait = !(filp->f_flags & O_NONBLOCK); + if (wait && cmpxchg(¤t_waiting_reader, NULL, current) != NULL) { + err = -EINTR; + goto out_unlock_reading; + } bytes_read = rng_get_data(rng, rng_buffer, - rng_buffer_size(), - !(filp->f_flags & O_NONBLOCK)); + rng_buffer_size(), wait); + if (wait && cmpxchg(¤t_waiting_reader, current, NULL) != current) + synchronize_rcu(); if (bytes_read < 0) { err = bytes_read; goto out_unlock_reading; @@ -513,8 +522,9 @@ static int hwrng_fillfn(void *unused) break;
if (rc <= 0) { - pr_warn("hwrng: no data available\n"); - msleep_interruptible(10000); + if (kthread_should_stop()) + break; + schedule_timeout_interruptible(HZ * 10); continue; }
@@ -608,13 +618,21 @@ int hwrng_register(struct hwrng *rng) } EXPORT_SYMBOL_GPL(hwrng_register);
+#define UNREGISTERING_READER ((void *)~0UL) + void hwrng_unregister(struct hwrng *rng) { struct hwrng *old_rng, *new_rng; + struct task_struct *waiting_reader; int err;
mutex_lock(&rng_mutex);
+ rcu_read_lock(); + waiting_reader = xchg(¤t_waiting_reader, UNREGISTERING_READER); + if (waiting_reader && waiting_reader != UNREGISTERING_READER) + set_notify_signal(waiting_reader); + rcu_read_unlock(); old_rng = current_rng; list_del(&rng->list); if (current_rng == rng) { @@ -640,6 +658,10 @@ void hwrng_unregister(struct hwrng *rng) }
wait_for_completion(&rng->cleanup_done); + + mutex_lock(&rng_mutex); + cmpxchg(¤t_waiting_reader, UNREGISTERING_READER, NULL); + mutex_unlock(&rng_mutex); } EXPORT_SYMBOL_GPL(hwrng_unregister);
diff --git a/drivers/net/wireless/ath/ath9k/rng.c b/drivers/net/wireless/ath/ath9k/rng.c index cb5414265a9b..8980dc36509e 100644 --- a/drivers/net/wireless/ath/ath9k/rng.c +++ b/drivers/net/wireless/ath/ath9k/rng.c @@ -52,18 +52,13 @@ static int ath9k_rng_data_read(struct ath_softc *sc, u32 *buf, u32 buf_size) return j << 2; }
-static u32 ath9k_rng_delay_get(u32 fail_stats) +static unsigned long ath9k_rng_delay_get(u32 fail_stats) { - u32 delay; - if (fail_stats < 100) - delay = 10; + return HZ / 100; else if (fail_stats < 105) - delay = 1000; - else - delay = 10000; - - return delay; + return HZ; + return HZ * 10; }
static int ath9k_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait) @@ -80,10 +75,10 @@ static int ath9k_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait) bytes_read += max & 3UL; memzero_explicit(&word, sizeof(word)); } - if (!wait || !max || likely(bytes_read) || fail_stats > 110) + if (!wait || !max || likely(bytes_read) || fail_stats > 110 || + ((current->flags & PF_KTHREAD) && kthread_should_stop()) || + schedule_timeout_interruptible(ath9k_rng_delay_get(++fail_stats))) break; - - msleep_interruptible(ath9k_rng_delay_get(++fail_stats)); }
if (wait && !bytes_read && max)
Hmm, set_notify_signal() calls wake_up_state() in kernel/sched/core.c, which is not currently exported. Only by including EXPORT_SYMBOL(wake_up_state) and rebuilding vmlinux was I able to build rng-core.ko and load it successfully.
That said, this patch allows 'ip link set wlan0 down' to wake a blocked process reading from /dev/hwrng, eliminating the delay as described. I'll give my sign-off with the EXPORT_SYMBOL sorted out.
Hi Gregory,
On Tue, Jun 28, 2022 at 08:41:44PM -0700, Gregory Erwin wrote:
Hmm, set_notify_signal() calls wake_up_state() in kernel/sched/core.c, which is not currently exported. Only by including EXPORT_SYMBOL(wake_up_state) and rebuilding vmlinux was I able to build rng-core.ko and load it successfully.
That said, this patch allows 'ip link set wlan0 down' to wake a blocked process reading from /dev/hwrng, eliminating the delay as described. I'll give my sign-off with the EXPORT_SYMBOL sorted out.
Thanks for testing, and thanks for the note about EXPORT_SYMBOL(). I'll send a v+1 with that fixed. And then it sounds like this patch finally addresses all the issues you were seeing. Pfiew!
Jason
There are two deadlock scenarios that need addressing, which cause problems when the computer goes to sleep, the interface is set down, and hwrng_unregister() is called. When the deadlock is hit, sleep is delayed for tens of seconds, causing it to fail. These scenarios are:
1) The hwrng kthread can't be stopped while it's sleeping, because it uses msleep_interruptible() instead of schedule_timeout_interruptible(). The fix is a simple moving to the correct function. At the same time, we should cleanup a common and useless dmesg splat in the same area.
2) A normal user thread can't be interrupted by hwrng_unregister() while it's sleeping, because hwrng_unregister() is called from elsewhere. The solution here is to keep track of which thread is currently reading, and asleep, and signal that thread when it's time to unregister. There's a bit of book keeping required to prevent lifetime issues on current.
Reported-by: Gregory Erwin gregerwin256@gmail.com Cc: Toke Høiland-Jørgensen toke@redhat.com Cc: Kalle Valo kvalo@kernel.org Cc: Rui Salvaterra rsalvaterra@gmail.com Cc: Herbert Xu herbert@gondor.apana.org.au Cc: stable@vger.kernel.org Fixes: fcd09c90c3c5 ("ath9k: use hw_random API instead of directly dumping into random.c") Link: https://lore.kernel.org/all/CAO+Okf6ZJC5-nTE_EJUGQtd8JiCkiEHytGgDsFGTEjs0c00... Link: https://lore.kernel.org/lkml/CAO+Okf5k+C+SE6pMVfPf-d8MfVPVq4PO7EY8Hys_DVXten... Link: https://bugs.archlinux.org/task/75138 Signed-off-by: Jason A. Donenfeld Jason@zx2c4.com --- Changes v7->v8: - Add a missing export_symbol.
drivers/char/hw_random/core.c | 30 ++++++++++++++++++++++++---- drivers/net/wireless/ath/ath9k/rng.c | 19 +++++++----------- kernel/sched/core.c | 1 + 3 files changed, 34 insertions(+), 16 deletions(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c index 16f227b995e8..df45c265878e 100644 --- a/drivers/char/hw_random/core.c +++ b/drivers/char/hw_random/core.c @@ -38,6 +38,8 @@ static LIST_HEAD(rng_list); static DEFINE_MUTEX(rng_mutex); /* Protects rng read functions, data_avail, rng_buffer and rng_fillbuf */ static DEFINE_MUTEX(reading_mutex); +/* Keeps track of whoever is wait-reading it currently while holding reading_mutex. */ +static struct task_struct *current_waiting_reader; static int data_avail; static u8 *rng_buffer, *rng_fillbuf; static unsigned short current_quality; @@ -208,6 +210,7 @@ static ssize_t rng_dev_read(struct file *filp, char __user *buf, int err = 0; int bytes_read, len; struct hwrng *rng; + bool wait;
while (size) { rng = get_current_rng(); @@ -225,9 +228,15 @@ static ssize_t rng_dev_read(struct file *filp, char __user *buf, goto out_put; } if (!data_avail) { + wait = !(filp->f_flags & O_NONBLOCK); + if (wait && cmpxchg(¤t_waiting_reader, NULL, current) != NULL) { + err = -EINTR; + goto out_unlock_reading; + } bytes_read = rng_get_data(rng, rng_buffer, - rng_buffer_size(), - !(filp->f_flags & O_NONBLOCK)); + rng_buffer_size(), wait); + if (wait && cmpxchg(¤t_waiting_reader, current, NULL) != current) + synchronize_rcu(); if (bytes_read < 0) { err = bytes_read; goto out_unlock_reading; @@ -513,8 +522,9 @@ static int hwrng_fillfn(void *unused) break;
if (rc <= 0) { - pr_warn("hwrng: no data available\n"); - msleep_interruptible(10000); + if (kthread_should_stop()) + break; + schedule_timeout_interruptible(HZ * 10); continue; }
@@ -608,13 +618,21 @@ int hwrng_register(struct hwrng *rng) } EXPORT_SYMBOL_GPL(hwrng_register);
+#define UNREGISTERING_READER ((void *)~0UL) + void hwrng_unregister(struct hwrng *rng) { struct hwrng *old_rng, *new_rng; + struct task_struct *waiting_reader; int err;
mutex_lock(&rng_mutex);
+ rcu_read_lock(); + waiting_reader = xchg(¤t_waiting_reader, UNREGISTERING_READER); + if (waiting_reader && waiting_reader != UNREGISTERING_READER) + set_notify_signal(waiting_reader); + rcu_read_unlock(); old_rng = current_rng; list_del(&rng->list); if (current_rng == rng) { @@ -640,6 +658,10 @@ void hwrng_unregister(struct hwrng *rng) }
wait_for_completion(&rng->cleanup_done); + + mutex_lock(&rng_mutex); + cmpxchg(¤t_waiting_reader, UNREGISTERING_READER, NULL); + mutex_unlock(&rng_mutex); } EXPORT_SYMBOL_GPL(hwrng_unregister);
diff --git a/drivers/net/wireless/ath/ath9k/rng.c b/drivers/net/wireless/ath/ath9k/rng.c index cb5414265a9b..8980dc36509e 100644 --- a/drivers/net/wireless/ath/ath9k/rng.c +++ b/drivers/net/wireless/ath/ath9k/rng.c @@ -52,18 +52,13 @@ static int ath9k_rng_data_read(struct ath_softc *sc, u32 *buf, u32 buf_size) return j << 2; }
-static u32 ath9k_rng_delay_get(u32 fail_stats) +static unsigned long ath9k_rng_delay_get(u32 fail_stats) { - u32 delay; - if (fail_stats < 100) - delay = 10; + return HZ / 100; else if (fail_stats < 105) - delay = 1000; - else - delay = 10000; - - return delay; + return HZ; + return HZ * 10; }
static int ath9k_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait) @@ -80,10 +75,10 @@ static int ath9k_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait) bytes_read += max & 3UL; memzero_explicit(&word, sizeof(word)); } - if (!wait || !max || likely(bytes_read) || fail_stats > 110) + if (!wait || !max || likely(bytes_read) || fail_stats > 110 || + ((current->flags & PF_KTHREAD) && kthread_should_stop()) || + schedule_timeout_interruptible(ath9k_rng_delay_get(++fail_stats))) break; - - msleep_interruptible(ath9k_rng_delay_get(++fail_stats)); }
if (wait && !bytes_read && max) diff --git a/kernel/sched/core.c b/kernel/sched/core.c index da0bf6fe9ecd..d65a5eb9a65e 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -4284,6 +4284,7 @@ int wake_up_state(struct task_struct *p, unsigned int state) { return try_to_wake_up(p, state, 0); } +EXPORT_SYMBOL(wake_up_state);
/* * Perform scheduler related setup for a newly forked process p.
On Wed, Jun 29, 2022 at 01:42:40PM +0200, Jason A. Donenfeld wrote:
--- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -4284,6 +4284,7 @@ int wake_up_state(struct task_struct *p, unsigned int state) { return try_to_wake_up(p, state, 0); } +EXPORT_SYMBOL(wake_up_state);
Should be EXPORT_SYMBOL_GPL(), right?
thanks,
greg k-h
On Wed, Jun 29, 2022 at 5:28 PM Greg KH gregkh@linuxfoundation.org wrote:
On Wed, Jun 29, 2022 at 01:42:40PM +0200, Jason A. Donenfeld wrote:
--- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -4284,6 +4284,7 @@ int wake_up_state(struct task_struct *p, unsigned int state) { return try_to_wake_up(p, state, 0); } +EXPORT_SYMBOL(wake_up_state);
Should be EXPORT_SYMBOL_GPL(), right?
The highly similar wake_up_process() above it, which has the exact same body, except the `state` argument is fixed as TASK_NORMAL, is an EXPORT_SYMBOL(). So I figured this one should follow form. Let me know if that's silly, and I'll send a v+1 changing it to _GPL though.
Jason
On Wed, Jun 29, 2022 at 06:15:49PM +0200, Jason A. Donenfeld wrote:
On Wed, Jun 29, 2022 at 5:28 PM Greg KH gregkh@linuxfoundation.org wrote:
On Wed, Jun 29, 2022 at 01:42:40PM +0200, Jason A. Donenfeld wrote:
--- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -4284,6 +4284,7 @@ int wake_up_state(struct task_struct *p, unsigned int state) { return try_to_wake_up(p, state, 0); } +EXPORT_SYMBOL(wake_up_state);
Should be EXPORT_SYMBOL_GPL(), right?
The highly similar wake_up_process() above it, which has the exact same body, except the `state` argument is fixed as TASK_NORMAL, is an EXPORT_SYMBOL(). So I figured this one should follow form. Let me know if that's silly, and I'll send a v+1 changing it to _GPL though.
I'll let the maintainers of this code decide that, I wasn't aware of the other symbol above this. It's their call, as it's their code.
thanks,
greg k-h
Hey Gregory,
On Wed, Jun 29, 2022 at 01:42:40PM +0200, Jason A. Donenfeld wrote:
There are two deadlock scenarios that need addressing, which cause problems when the computer goes to sleep, the interface is set down, and hwrng_unregister() is called. When the deadlock is hit, sleep is delayed for tens of seconds, causing it to fail. These scenarios are:
The hwrng kthread can't be stopped while it's sleeping, because it uses msleep_interruptible() instead of schedule_timeout_interruptible(). The fix is a simple moving to the correct function. At the same time, we should cleanup a common and useless dmesg splat in the same area.
A normal user thread can't be interrupted by hwrng_unregister() while it's sleeping, because hwrng_unregister() is called from elsewhere. The solution here is to keep track of which thread is currently reading, and asleep, and signal that thread when it's time to unregister. There's a bit of book keeping required to prevent lifetime issues on current.
Reported-by: Gregory Erwin gregerwin256@gmail.com Cc: Toke Høiland-Jørgensen toke@redhat.com Cc: Kalle Valo kvalo@kernel.org Cc: Rui Salvaterra rsalvaterra@gmail.com Cc: Herbert Xu herbert@gondor.apana.org.au Cc: stable@vger.kernel.org Fixes: fcd09c90c3c5 ("ath9k: use hw_random API instead of directly dumping into random.c") Link: https://lore.kernel.org/all/CAO+Okf6ZJC5-nTE_EJUGQtd8JiCkiEHytGgDsFGTEjs0c00... Link: https://lore.kernel.org/lkml/CAO+Okf5k+C+SE6pMVfPf-d8MfVPVq4PO7EY8Hys_DVXten... Link: https://bugs.archlinux.org/task/75138 Signed-off-by: Jason A. Donenfeld Jason@zx2c4.com
Hoping for your `Tested-by:` if this still works for you.
Jason
On Thu, Jun 30, 2022 at 7:03 AM Jason A. Donenfeld Jason@zx2c4.com wrote:
Hey Gregory,
On Wed, Jun 29, 2022 at 01:42:40PM +0200, Jason A. Donenfeld wrote:
There are two deadlock scenarios that need addressing, which cause problems when the computer goes to sleep, the interface is set down, and hwrng_unregister() is called. When the deadlock is hit, sleep is delayed for tens of seconds, causing it to fail. These scenarios are:
The hwrng kthread can't be stopped while it's sleeping, because it uses msleep_interruptible() instead of schedule_timeout_interruptible(). The fix is a simple moving to the correct function. At the same time, we should cleanup a common and useless dmesg splat in the same area.
A normal user thread can't be interrupted by hwrng_unregister() while it's sleeping, because hwrng_unregister() is called from elsewhere. The solution here is to keep track of which thread is currently reading, and asleep, and signal that thread when it's time to unregister. There's a bit of book keeping required to prevent lifetime issues on current.
Reported-by: Gregory Erwin gregerwin256@gmail.com Cc: Toke Høiland-Jørgensen toke@redhat.com Cc: Kalle Valo kvalo@kernel.org Cc: Rui Salvaterra rsalvaterra@gmail.com Cc: Herbert Xu herbert@gondor.apana.org.au Cc: stable@vger.kernel.org Fixes: fcd09c90c3c5 ("ath9k: use hw_random API instead of directly dumping into random.c") Link: https://lore.kernel.org/all/CAO+Okf6ZJC5-nTE_EJUGQtd8JiCkiEHytGgDsFGTEjs0c00... Link: https://lore.kernel.org/lkml/CAO+Okf5k+C+SE6pMVfPf-d8MfVPVq4PO7EY8Hys_DVXten... Link: https://bugs.archlinux.org/task/75138 Signed-off-by: Jason A. Donenfeld Jason@zx2c4.com
Hoping for your `Tested-by:` if this still works for you.
Jason
Apologies for the delay. Tested-by: Gregory Erwin gregerwin256@gmail.com
"Jason A. Donenfeld" Jason@zx2c4.com writes:
There are two deadlock scenarios that need addressing, which cause problems when the computer goes to sleep, the interface is set down, and hwrng_unregister() is called. When the deadlock is hit, sleep is delayed for tens of seconds, causing it to fail. These scenarios are:
The hwrng kthread can't be stopped while it's sleeping, because it uses msleep_interruptible() instead of schedule_timeout_interruptible(). The fix is a simple moving to the correct function. At the same time, we should cleanup a common and useless dmesg splat in the same area.
A normal user thread can't be interrupted by hwrng_unregister() while it's sleeping, because hwrng_unregister() is called from elsewhere. The solution here is to keep track of which thread is currently reading, and asleep, and signal that thread when it's time to unregister. There's a bit of book keeping required to prevent lifetime issues on current.
Reported-by: Gregory Erwin gregerwin256@gmail.com Cc: Toke Høiland-Jørgensen toke@redhat.com Cc: Kalle Valo kvalo@kernel.org Cc: Rui Salvaterra rsalvaterra@gmail.com Cc: Herbert Xu herbert@gondor.apana.org.au Cc: stable@vger.kernel.org Fixes: fcd09c90c3c5 ("ath9k: use hw_random API instead of directly dumping into random.c") Link: https://lore.kernel.org/all/CAO+Okf6ZJC5-nTE_EJUGQtd8JiCkiEHytGgDsFGTEjs0c00... Link: https://lore.kernel.org/lkml/CAO+Okf5k+C+SE6pMVfPf-d8MfVPVq4PO7EY8Hys_DVXten... Link: https://bugs.archlinux.org/task/75138 Signed-off-by: Jason A. Donenfeld Jason@zx2c4.com
With the change to EXPORT_SYMBOL_GPL() for wake_up_state that Kalle has kindly agreed to fix up while applying:
Acked-by: Toke Høiland-Jørgensen toke@toke.dk
"Jason A. Donenfeld" Jason@zx2c4.com writes:
There are two deadlock scenarios that need addressing, which cause problems when the computer goes to sleep, the interface is set down, and hwrng_unregister() is called. When the deadlock is hit, sleep is delayed for tens of seconds, causing it to fail. These scenarios are:
The hwrng kthread can't be stopped while it's sleeping, because it uses msleep_interruptible() instead of schedule_timeout_interruptible(). The fix is a simple moving to the correct function. At the same time, we should cleanup a common and useless dmesg splat in the same area.
A normal user thread can't be interrupted by hwrng_unregister() while it's sleeping, because hwrng_unregister() is called from elsewhere. The solution here is to keep track of which thread is currently reading, and asleep, and signal that thread when it's time to unregister. There's a bit of book keeping required to prevent lifetime issues on current.
Reported-by: Gregory Erwin gregerwin256@gmail.com Cc: Toke Høiland-Jørgensen toke@redhat.com Cc: Kalle Valo kvalo@kernel.org Cc: Rui Salvaterra rsalvaterra@gmail.com Cc: Herbert Xu herbert@gondor.apana.org.au Cc: stable@vger.kernel.org Fixes: fcd09c90c3c5 ("ath9k: use hw_random API instead of directly dumping into random.c") Link: https://lore.kernel.org/all/CAO+Okf6ZJC5-nTE_EJUGQtd8JiCkiEHytGgDsFGTEjs0c00... Link: https://lore.kernel.org/lkml/CAO+Okf5k+C+SE6pMVfPf-d8MfVPVq4PO7EY8Hys_DVXten... Link: https://bugs.archlinux.org/task/75138 Signed-off-by: Jason A. Donenfeld Jason@zx2c4.com
drivers/char/hw_random/core.c | 30 ++++++++++++++++++++++++---- drivers/net/wireless/ath/ath9k/rng.c | 19 +++++++----------- 2 files changed, 33 insertions(+), 16 deletions(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c index 16f227b995e8..df45c265878e 100644 --- a/drivers/char/hw_random/core.c +++ b/drivers/char/hw_random/core.c @@ -38,6 +38,8 @@ static LIST_HEAD(rng_list); static DEFINE_MUTEX(rng_mutex); /* Protects rng read functions, data_avail, rng_buffer and rng_fillbuf */ static DEFINE_MUTEX(reading_mutex); +/* Keeps track of whoever is wait-reading it currently while holding reading_mutex. */ +static struct task_struct *current_waiting_reader; static int data_avail; static u8 *rng_buffer, *rng_fillbuf; static unsigned short current_quality; @@ -208,6 +210,7 @@ static ssize_t rng_dev_read(struct file *filp, char __user *buf, int err = 0; int bytes_read, len; struct hwrng *rng;
- bool wait;
while (size) { rng = get_current_rng(); @@ -225,9 +228,15 @@ static ssize_t rng_dev_read(struct file *filp, char __user *buf, goto out_put; } if (!data_avail) {
wait = !(filp->f_flags & O_NONBLOCK);if (wait && cmpxchg(¤t_waiting_reader, NULL, current) != NULL) {err = -EINTR;goto out_unlock_reading;} bytes_read = rng_get_data(rng, rng_buffer,
rng_buffer_size(),!(filp->f_flags & O_NONBLOCK));
rng_buffer_size(), wait);if (wait && cmpxchg(¤t_waiting_reader, current, NULL) != current)synchronize_rcu();
So this synchronize_rcu() is to ensure the hwrng_unregister() thread has exited the rcu_read_lock() section below? Isn't that a bit... creative... use of RCU? :)
Also, synchronize_rcu() can potentially take a while on a busy system, is it OK to call it while holding the mutex?
-Toke
Hi Toke,
On Wed, Jun 29, 2022 at 11:24:49AM +0200, Toke Høiland-Jørgensen wrote:
wait = !(filp->f_flags & O_NONBLOCK);if (wait && cmpxchg(¤t_waiting_reader, NULL, current) != NULL) {err = -EINTR;goto out_unlock_reading;} bytes_read = rng_get_data(rng, rng_buffer,
rng_buffer_size(),!(filp->f_flags & O_NONBLOCK));
rng_buffer_size(), wait);if (wait && cmpxchg(¤t_waiting_reader, current, NULL) != current)synchronize_rcu();So this synchronize_rcu() is to ensure the hwrng_unregister() thread has exited the rcu_read_lock() section below? Isn't that a bit... creative... use of RCU? :)
It's to handle the extreeeeemely unlikely race in which hwrng_unregister() does its xchg, and then the thread calling rng_dev_read() entirely exits. In practice, the only way I'm able to trigger this race is by synthetically adding `msleep()` in the right spot. But anyway, for that reason, it's only synchronized if that second cmpxchg indicates that indeed the value was changed out from under us.
Also, synchronize_rcu() can potentially take a while on a busy system, is it OK to call it while holding the mutex?
The reading mutex won't be usable by anything anyway at this point, so I don't think it matters.
Jason
linux-stable-mirror@lists.linaro.org
-
Greg KH -
Gregory Erwin -
Jason A. Donenfeld -
Toke Høiland-Jørgensen -
Toke Høiland-Jørgensen