On Mon, Jul 25, 2022 at 11:55:36PM +0200, Jason A. Donenfeld wrote:
Kalle - I'm resending this to you with the various acks collected from the long sprawling thread, in hopes that this actually makes it into your tree. Please let me know if you *don't* want to take this. -Jason
Hi Jason:
Thanks for all your effort in resolving this issue.
I think Valentin's concern is valid though. The sleep/wakeup paradigm in this patch-set is slightly unusual.
So what I've done is taken your latest patch, and incorporated Valentin's suggestions on top of it. I don't think there is an issue with other drivers as neither approach really changes them.
While doing so I've found a little problem with your patch.
@@ -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);
In between the setting and clearing of current_waiting_reader, a reader that gets a new RNG may fail simply because it detected the value of UNREGITERING_READER.
So I've fixed this by using a different mechanism to detect whether an RNG is going away, namely list_empty on rng->list. In doing so we no longer need to modify current_waiting_reader in hwrng_unregister and therefore it no longer needs to be modified using xchg.
---8<--- From: Jason A. Donenfeld Jason@zx2c4.com
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(). 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.
Cc: stable@vger.kernel.org Reported-by: Gregory Erwin gregerwin256@gmail.com 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 Signed-off-by: Herbert Xu herbert@gondor.apana.org.au
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c index 16f227b995e8..4f254b65ce0b 100644 --- a/drivers/char/hw_random/core.c +++ b/drivers/char/hw_random/core.c @@ -10,6 +10,7 @@ * of the GNU General Public License, incorporated herein by reference. */
+#include <linux/atomic.h> #include <linux/delay.h> #include <linux/device.h> #include <linux/err.h> @@ -22,6 +23,7 @@ #include <linux/miscdevice.h> #include <linux/module.h> #include <linux/random.h> +#include <linux/rcupdate.h> #include <linux/sched.h> #include <linux/slab.h> #include <linux/uaccess.h> @@ -38,6 +40,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; @@ -57,6 +61,23 @@ static void hwrng_manage_rngd(struct hwrng *rng); static inline int rng_get_data(struct hwrng *rng, u8 *buffer, size_t size, int wait);
+/* rng_dying without a barrier. Use this if there is a barrier elsewhere. */ +static inline bool __rng_dying(struct hwrng *rng) +{ + return list_empty(&rng->list); +} + +static inline bool rng_dying(struct hwrng *rng) +{ + /* + * This barrier pairs with the one in + * hwrng_unregister. This ensures that + * we see any attempt to unregister rng. + */ + smp_mb(); + return list_empty(&rng->list); +} + static size_t rng_buffer_size(void) { return SMP_CACHE_BYTES < 32 ? 32 : SMP_CACHE_BYTES; @@ -204,6 +225,7 @@ static inline int rng_get_data(struct hwrng *rng, u8 *buffer, size_t size, static ssize_t rng_dev_read(struct file *filp, char __user *buf, size_t size, loff_t *offp) { + int synch = false; ssize_t ret = 0; int err = 0; int bytes_read, len; @@ -225,9 +247,16 @@ static ssize_t rng_dev_read(struct file *filp, char __user *buf, goto out_put; } if (!data_avail) { + bool wait = !(filp->f_flags & O_NONBLOCK); + + if (wait) + current_waiting_reader = current; bytes_read = rng_get_data(rng, rng_buffer, - rng_buffer_size(), - !(filp->f_flags & O_NONBLOCK)); + rng_buffer_size(), wait); + if (wait) { + current_waiting_reader = NULL; + synch |= rng_dying(rng); + } if (bytes_read < 0) { err = bytes_read; goto out_unlock_reading; @@ -269,6 +298,9 @@ static ssize_t rng_dev_read(struct file *filp, char __user *buf, } } out: + if (synch) + synchronize_rcu(); + return ret ? : err;
out_unlock_reading: @@ -501,20 +533,26 @@ static int hwrng_fillfn(void *unused) if (IS_ERR(rng) || !rng) break; mutex_lock(&reading_mutex); + current_waiting_reader = current; rc = rng_get_data(rng, rng_fillbuf, rng_buffer_size(), 1); + current_waiting_reader = NULL; if (current_quality != rng->quality) rng->quality = current_quality; /* obsolete */ quality = rng->quality; mutex_unlock(&reading_mutex); + if (rng_dying(rng)) + synchronize_rcu(); put_rng(rng);
if (!quality) break;
if (rc <= 0) { - pr_warn("hwrng: no data available\n"); - msleep_interruptible(10000); + set_current_state(TASK_INTERRUPTIBLE); + if (kthread_should_stop()) + __set_current_state(TASK_RUNNING); + schedule_timeout(10 * HZ); continue; }
@@ -616,8 +654,22 @@ void hwrng_unregister(struct hwrng *rng) mutex_lock(&rng_mutex);
old_rng = current_rng; - list_del(&rng->list); + list_del_init(&rng->list); if (current_rng == rng) { + struct task_struct *waiting_reader; + + /* + * Ensure that rng->list is cleared before the + * subsequent read of current_waiting_reader. + */ + smp_mb(); + + rcu_read_lock(); + waiting_reader = current_waiting_reader; + if (waiting_reader) + wake_up_process(waiting_reader); + rcu_read_unlock(); + err = enable_best_rng(); if (err) { drop_current_rng(); @@ -685,6 +737,17 @@ void devm_hwrng_unregister(struct device *dev, struct hwrng *rng) } EXPORT_SYMBOL_GPL(devm_hwrng_unregister);
+long hwrng_msleep(struct hwrng *rng, unsigned int msecs) +{ + set_current_state(TASK_INTERRUPTIBLE); + + if (__rng_dying(rng)) + __set_current_state(TASK_RUNNING); + + return schedule_timeout(msecs); +} +EXPORT_SYMBOL_GPL(hwrng_msleep); + static int __init hwrng_modinit(void) { int ret; diff --git a/drivers/net/wireless/ath/ath9k/rng.c b/drivers/net/wireless/ath/ath9k/rng.c index cb5414265a9b..58c0ab01771b 100644 --- a/drivers/net/wireless/ath/ath9k/rng.c +++ b/drivers/net/wireless/ath/ath9k/rng.c @@ -83,7 +83,8 @@ static int ath9k_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait) if (!wait || !max || likely(bytes_read) || fail_stats > 110) break;
- msleep_interruptible(ath9k_rng_delay_get(++fail_stats)); + if (hwrng_msleep(rng, ath9k_rng_delay_get(++fail_stats))) + break; }
if (wait && !bytes_read && max) diff --git a/include/linux/hw_random.h b/include/linux/hw_random.h index aa1d4da03538..021a971bfd68 100644 --- a/include/linux/hw_random.h +++ b/include/linux/hw_random.h @@ -61,4 +61,6 @@ extern int devm_hwrng_register(struct device *dev, struct hwrng *rng); extern void hwrng_unregister(struct hwrng *rng); extern void devm_hwrng_unregister(struct device *dve, struct hwrng *rng);
+extern long hwrng_msleep(struct hwrng *rng, unsigned int msecs); + #endif /* LINUX_HWRANDOM_H_ */