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@xxxxxxxxx>
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@xxxxxxxxxxxxxxx>
Reported-by: Gregory Erwin <gregerwin256@xxxxxxxxx>
Fixes: fcd09c90c3c5 ("ath9k: use hw_random API instead of directly dumping into random.c")
Link: https://lore.kernel.org/all/CAO+Okf6ZJC5-nTE_EJUGQtd8JiCkiEHytGgDsFGTEjs0c00giw@xxxxxxxxxxxxxx/
Link: https://lore.kernel.org/lkml/CAO+Okf5k+C+SE6pMVfPf-d8MfVPVq4PO7EY8Hys_DVXtent3HA@xxxxxxxxxxxxxx/
Link: https://bugs.archlinux.org/task/75138
Signed-off-by: Jason A. Donenfeld <Jason@xxxxxxxxx>
Signed-off-by: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
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_ */
--
Email: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
