[PATCH RESEND v11] hwrng: core - let sleep be interrupted when unregistering hwrng

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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(&current_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(&current_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



[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux