[PATCH v7] ath9k: let sleep be interrupted when unregistering hwrng

[PATCH v7] ath9k: let sleep be interrupted when unregistering hwrng

[Jason A. Donenfeld]

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_EJUGQtd8JiCkiEHytGgDsFGTEjs0c00giw@mail.gmail.com/
Link: https://lore.kernel.org/lkml/CAO+Okf5k+C+SE6pMVfPf-d8MfVPVq4PO7EY8Hys_DVXtent3HA@mail.gmail.com/
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(&current_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(&current_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(&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);
 }
 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)
-- 
2.35.1

Re: [PATCH v7] ath9k: let sleep be interrupted when unregistering hwrng

[Gregory Erwin]

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.

Re: [PATCH v7] ath9k: let sleep be interrupted when unregistering hwrng

[Toke Høiland-Jørgensen]

"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:
>
> 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_EJUGQtd8JiCkiEHytGgDsFGTEjs0c00giw@mail.gmail.com/
> Link: https://lore.kernel.org/lkml/CAO+Okf5k+C+SE6pMVfPf-d8MfVPVq4PO7EY8Hys_DVXtent3HA@mail.gmail.com/
> 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(&current_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(&current_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

Re: [PATCH v7] ath9k: let sleep be interrupted when unregistering hwrng

[Jason A. Donenfeld]

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

Re: [PATCH v7] ath9k: let sleep be interrupted when unregistering hwrng

[Jason A. Donenfeld]

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

[PATCH v8] ath9k: let sleep be interrupted when unregistering hwrng

[Jason A. Donenfeld]

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_EJUGQtd8JiCkiEHytGgDsFGTEjs0c00giw@mail.gmail.com/
Link: https://lore.kernel.org/lkml/CAO+Okf5k+C+SE6pMVfPf-d8MfVPVq4PO7EY8Hys_DVXtent3HA@mail.gmail.com/
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(&current_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(&current_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(&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);
 }
 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.
-- 
2.35.1

Re: [PATCH v8] ath9k: let sleep be interrupted when unregistering hwrng

[Greg KH]

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

Re: [PATCH v8] ath9k: let sleep be interrupted when unregistering hwrng

[Jason A. Donenfeld]

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

Re: [PATCH v8] ath9k: let sleep be interrupted when unregistering hwrng

[Greg KH]

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

Re: [PATCH v8] ath9k: let sleep be interrupted when unregistering hwrng

[Jason A. Donenfeld]

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:
> 
> 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_EJUGQtd8JiCkiEHytGgDsFGTEjs0c00giw@mail.gmail.com/
> Link: https://lore.kernel.org/lkml/CAO+Okf5k+C+SE6pMVfPf-d8MfVPVq4PO7EY8Hys_DVXtent3HA@mail.gmail.com/
> 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

Re: [PATCH v8] ath9k: let sleep be interrupted when unregistering hwrng

[Gregory Erwin]

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:
> >
> > 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_EJUGQtd8JiCkiEHytGgDsFGTEjs0c00giw@mail.gmail.com/
> > Link: https://lore.kernel.org/lkml/CAO+Okf5k+C+SE6pMVfPf-d8MfVPVq4PO7EY8Hys_DVXtent3HA@mail.gmail.com/
> > 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>

Re: [PATCH v8] ath9k: let sleep be interrupted when unregistering hwrng

[Toke Høiland-Jørgensen]

"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:
>
> 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_EJUGQtd8JiCkiEHytGgDsFGTEjs0c00giw@mail.gmail.com/
> Link: https://lore.kernel.org/lkml/CAO+Okf5k+C+SE6pMVfPf-d8MfVPVq4PO7EY8Hys_DVXtent3HA@mail.gmail.com/
> 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>

Re: [PATCH v8] ath9k: let sleep be interrupted when unregistering hwrng

[Kalle Valo]

"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:
>
> 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_EJUGQtd8JiCkiEHytGgDsFGTEjs0c00giw@mail.gmail.com/
> Link: https://lore.kernel.org/lkml/CAO+Okf5k+C+SE6pMVfPf-d8MfVPVq4PO7EY8Hys_DVXtent3HA@mail.gmail.com/
> 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(-)

I don't see any acks for the hw_random and the scheduler change, adding more
people to CC. Full patch here:

https://patchwork.kernel.org/project/linux-wireless/patch/20220629114240.946411-1-Jason@zx2c4.com/

Are everyone ok if I take this patch via wireless-next?

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH v8] ath9k: let sleep be interrupted when unregistering hwrng

[Valentin Schneider]

On 07/07/22 19:26, Kalle Valo wrote:
> "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:
>>
>> 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_EJUGQtd8JiCkiEHytGgDsFGTEjs0c00giw@mail.gmail.com/
>> Link: https://lore.kernel.org/lkml/CAO+Okf5k+C+SE6pMVfPf-d8MfVPVq4PO7EY8Hys_DVXtent3HA@mail.gmail.com/
>> 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(-)
>
> I don't see any acks for the hw_random and the scheduler change, adding more
> people to CC. Full patch here:
>
> https://patchwork.kernel.org/project/linux-wireless/patch/20220629114240.946411-1-Jason@zx2c4.com/
>
> Are everyone ok if I take this patch via wireless-next?
>

Thanks for the Cc.

I'm not hot on the export of wake_up_state(), IMO any wakeup with
!(state & TASK_NORMAL) should be reserved to kernel internals. Now, here
IIUC the problem is that the patch uses an inline invoking

  wake_up_state(p, TASK_INTERRUPTIBLE)

so this isn't playing with any 'exotic' task state, thus it shouldn't
actually need the export.

I've been trying to figure out if this could work with just a
wake_up_process(), but the sleeping pattern here is not very conforming
(cf. 'wait loop' pattern in sched/core.c), AFAICT the signal is used to
circumvent that :/

Re: [PATCH v8] ath9k: let sleep be interrupted when unregistering hwrng

[Jason A. Donenfeld]

Hi Valentin,

On 7/11/22, Valentin Schneider <vschneid@redhat.com> wrote:
> On 07/07/22 19:26, Kalle Valo wrote:
>> "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:
>>>
>>> 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_EJUGQtd8JiCkiEHytGgDsFGTEjs0c00giw@mail.gmail.com/
>>> Link:
>>> https://lore.kernel.org/lkml/CAO+Okf5k+C+SE6pMVfPf-d8MfVPVq4PO7EY8Hys_DVXtent3HA@mail.gmail.com/
>>> 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(-)
>>
>> I don't see any acks for the hw_random and the scheduler change, adding
>> more
>> people to CC. Full patch here:
>>
>> https://patchwork.kernel.org/project/linux-wireless/patch/20220629114240.946411-1-Jason@zx2c4.com/
>>
>> Are everyone ok if I take this patch via wireless-next?
>>
>
> Thanks for the Cc.
>
> I'm not hot on the export of wake_up_state(), IMO any wakeup with
> !(state & TASK_NORMAL) should be reserved to kernel internals. Now, here
> IIUC the problem is that the patch uses an inline invoking
>
>   wake_up_state(p, TASK_INTERRUPTIBLE)
>
> so this isn't playing with any 'exotic' task state, thus it shouldn't
> actually need the export.
>
> I've been trying to figure out if this could work with just a
> wake_up_process(), but the sleeping pattern here is not very conforming
> (cf. 'wait loop' pattern in sched/core.c), AFAICT the signal is used to
> circumvent that :/

I don't intend to work on this patch more. If you'd like to ack the
trivial scheduler change (adding EXPORT_SYMBOL), that'd help, and then
this can move forward as planned. Otherwise, if you have particular
opinions about this patch that you want to happen, feel free to pick
up the patch and send your own revisions (though I don't intend to do
further review). Alternatively, I'll just send a patch to remove the
driver entirely. Hopefully you do find this ack-able, though.

Jason

Re: [PATCH v8] ath9k: let sleep be interrupted when unregistering hwrng

[Valentin Schneider]

On 11/07/22 13:53, Jason A. Donenfeld wrote:
> Hi Valentin,
>
> On 7/11/22, Valentin Schneider <vschneid@redhat.com> wrote:
>> Thanks for the Cc.
>>
>> I'm not hot on the export of wake_up_state(), IMO any wakeup with
>> !(state & TASK_NORMAL) should be reserved to kernel internals. Now, here
>> IIUC the problem is that the patch uses an inline invoking
>>
>>   wake_up_state(p, TASK_INTERRUPTIBLE)
>>
>> so this isn't playing with any 'exotic' task state, thus it shouldn't
>> actually need the export.
>>
>> I've been trying to figure out if this could work with just a
>> wake_up_process(), but the sleeping pattern here is not very conforming
>> (cf. 'wait loop' pattern in sched/core.c), AFAICT the signal is used to
>> circumvent that :/
>
> I don't intend to work on this patch more. If you'd like to ack the
> trivial scheduler change (adding EXPORT_SYMBOL), that'd help, and then
> this can move forward as planned. Otherwise, if you have particular
> opinions about this patch that you want to happen, feel free to pick
> up the patch and send your own revisions (though I don't intend to do
> further review). Alternatively, I'll just send a patch to remove the
> driver entirely. Hopefully you do find this ack-able, though.
>

I'm not for a blanket wake_up_state() export, however if we *really* need
it then I suppose we could have a wake_up_process_interruptible() exported
and used by __set_notify_signal().

Re: [PATCH v8] ath9k: let sleep be interrupted when unregistering hwrng

[Jason A. Donenfeld]

Hi Valentin,

On Tue, Jul 19, 2022 at 04:15:02PM +0100, Valentin Schneider wrote:
> On 11/07/22 13:53, Jason A. Donenfeld wrote:
> > Hi Valentin,
> >
> > On 7/11/22, Valentin Schneider <vschneid@redhat.com> wrote:
> >> Thanks for the Cc.
> >>
> >> I'm not hot on the export of wake_up_state(), IMO any wakeup with
> >> !(state & TASK_NORMAL) should be reserved to kernel internals. Now, here
> >> IIUC the problem is that the patch uses an inline invoking
> >>
> >>   wake_up_state(p, TASK_INTERRUPTIBLE)
> >>
> >> so this isn't playing with any 'exotic' task state, thus it shouldn't
> >> actually need the export.
> >>
> >> I've been trying to figure out if this could work with just a
> >> wake_up_process(), but the sleeping pattern here is not very conforming
> >> (cf. 'wait loop' pattern in sched/core.c), AFAICT the signal is used to
> >> circumvent that :/
> >
> > I don't intend to work on this patch more. If you'd like to ack the
> > trivial scheduler change (adding EXPORT_SYMBOL), that'd help, and then
> > this can move forward as planned. Otherwise, if you have particular
> > opinions about this patch that you want to happen, feel free to pick
> > up the patch and send your own revisions (though I don't intend to do
> > further review). Alternatively, I'll just send a patch to remove the
> > driver entirely. Hopefully you do find this ack-able, though.
> >
> 
> I'm not for a blanket wake_up_state() export, however if we *really* need
> it then I suppose we could have a wake_up_process_interruptible() exported
> and used by __set_notify_signal().
> 
Thanks for keeping this thread alive. I'll do what you suggest and send
a v+1. I think I understand the idea. Let's see how it goes.

Jason

[PATCH v9] ath9k: let sleep be interrupted when unregistering hwrng

[Jason A. Donenfeld]

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.

Cc: Kalle Valo <kvalo@kernel.org>
Cc: Rui Salvaterra <rsalvaterra@gmail.com>
Cc: Eric W. Biederman <ebiederm@xmission.com>
Cc: Valentin Schneider <vschneid@redhat.com>
Cc: stable@vger.kernel.org
Reported-by: Gregory Erwin <gregerwin256@gmail.com>
Tested-by: Gregory Erwin <gregerwin256@gmail.com>
Acked-by: Toke Høiland-Jørgensen <toke@toke.dk>
Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
Fixes: fcd09c90c3c5 ("ath9k: use hw_random API instead of directly dumping into random.c")
Link: https://lore.kernel.org/all/CAO+Okf6ZJC5-nTE_EJUGQtd8JiCkiEHytGgDsFGTEjs0c00giw@mail.gmail.com/
Link: https://lore.kernel.org/lkml/CAO+Okf5k+C+SE6pMVfPf-d8MfVPVq4PO7EY8Hys_DVXtent3HA@mail.gmail.com/
Link: https://bugs.archlinux.org/task/75138
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
Changes v8->v9:
- Use EXPORT_SYMBOL_GPL instead of EXPORT_SYMBOL.
- Don't export wake_up_state, but rather have __set_notify_signal use
  wake_up_process_interruptible.

 drivers/char/hw_random/core.c        | 30 ++++++++++++++++++++++++----
 drivers/net/wireless/ath/ath9k/rng.c | 19 +++++++-----------
 include/linux/sched.h                |  1 +
 include/linux/sched/signal.h         |  2 +-
 kernel/sched/core.c                  |  6 ++++++
 5 files changed, 41 insertions(+), 17 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(&current_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(&current_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(&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);
 }
 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/include/linux/sched.h b/include/linux/sched.h
index c46f3a63b758..518fb7694270 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1936,6 +1936,7 @@ extern struct task_struct *find_get_task_by_vpid(pid_t nr);
 
 extern int wake_up_state(struct task_struct *tsk, unsigned int state);
 extern int wake_up_process(struct task_struct *tsk);
+extern int wake_up_process_interruptible(struct task_struct *tsk);
 extern void wake_up_new_task(struct task_struct *tsk);
 
 #ifdef CONFIG_SMP
diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
index cafbe03eed01..e1c0099ba857 100644
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -364,7 +364,7 @@ static inline void clear_notify_signal(void)
 static inline bool __set_notify_signal(struct task_struct *task)
 {
 	return !test_and_set_tsk_thread_flag(task, TIF_NOTIFY_SIGNAL) &&
-	       !wake_up_state(task, TASK_INTERRUPTIBLE);
+	       !wake_up_process_interruptible(task);
 }
 
 /*
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index da0bf6fe9ecd..8e466f0d906f 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -4280,6 +4280,12 @@ int wake_up_process(struct task_struct *p)
 }
 EXPORT_SYMBOL(wake_up_process);
 
+int wake_up_process_interruptible(struct task_struct *p)
+{
+	return try_to_wake_up(p, TASK_INTERRUPTIBLE, 0);
+}
+EXPORT_SYMBOL_GPL(wake_up_process_interruptible);
+
 int wake_up_state(struct task_struct *p, unsigned int state)
 {
 	return try_to_wake_up(p, state, 0);
-- 
2.35.1

Re: [PATCH v9] ath9k: let sleep be interrupted when unregistering hwrng

[Eric W. Biederman]

"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:
>
> 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.

Is there any chance you can name the new function
wake_up_task_interruptible instead of wake_up_process_interruptible.

The name wake_up_process is wrong now, it does not wake up all threads
of a process.  The name dates back to before linux supported multiple
threads in a process, so it is grandfathered in until someone gets
changes it.  But please let's not have a new function with a incorrect
and confusing name.

Eric

>
> Cc: Kalle Valo <kvalo@kernel.org>
> Cc: Rui Salvaterra <rsalvaterra@gmail.com>
> Cc: Eric W. Biederman <ebiederm@xmission.com>
> Cc: Valentin Schneider <vschneid@redhat.com>
> Cc: stable@vger.kernel.org
> Reported-by: Gregory Erwin <gregerwin256@gmail.com>
> Tested-by: Gregory Erwin <gregerwin256@gmail.com>
> Acked-by: Toke Høiland-Jørgensen <toke@toke.dk>
> Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
> Fixes: fcd09c90c3c5 ("ath9k: use hw_random API instead of directly dumping into random.c")
> Link: https://lore.kernel.org/all/CAO+Okf6ZJC5-nTE_EJUGQtd8JiCkiEHytGgDsFGTEjs0c00giw@mail.gmail.com/
> Link: https://lore.kernel.org/lkml/CAO+Okf5k+C+SE6pMVfPf-d8MfVPVq4PO7EY8Hys_DVXtent3HA@mail.gmail.com/
> Link: https://bugs.archlinux.org/task/75138
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
> Changes v8->v9:
> - Use EXPORT_SYMBOL_GPL instead of EXPORT_SYMBOL.
> - Don't export wake_up_state, but rather have __set_notify_signal use
>   wake_up_process_interruptible.
>
>  drivers/char/hw_random/core.c        | 30 ++++++++++++++++++++++++----
>  drivers/net/wireless/ath/ath9k/rng.c | 19 +++++++-----------
>  include/linux/sched.h                |  1 +
>  include/linux/sched/signal.h         |  2 +-
>  kernel/sched/core.c                  |  6 ++++++
>  5 files changed, 41 insertions(+), 17 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(&current_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(&current_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(&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);
>  }
>  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/include/linux/sched.h b/include/linux/sched.h
> index c46f3a63b758..518fb7694270 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1936,6 +1936,7 @@ extern struct task_struct *find_get_task_by_vpid(pid_t nr);
>  
>  extern int wake_up_state(struct task_struct *tsk, unsigned int state);
>  extern int wake_up_process(struct task_struct *tsk);
> +extern int wake_up_process_interruptible(struct task_struct *tsk);
>  extern void wake_up_new_task(struct task_struct *tsk);
>  
>  #ifdef CONFIG_SMP
> diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
> index cafbe03eed01..e1c0099ba857 100644
> --- a/include/linux/sched/signal.h
> +++ b/include/linux/sched/signal.h
> @@ -364,7 +364,7 @@ static inline void clear_notify_signal(void)
>  static inline bool __set_notify_signal(struct task_struct *task)
>  {
>  	return !test_and_set_tsk_thread_flag(task, TIF_NOTIFY_SIGNAL) &&
> -	       !wake_up_state(task, TASK_INTERRUPTIBLE);
> +	       !wake_up_process_interruptible(task);
>  }
>  
>  /*
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index da0bf6fe9ecd..8e466f0d906f 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -4280,6 +4280,12 @@ int wake_up_process(struct task_struct *p)
>  }
>  EXPORT_SYMBOL(wake_up_process);
>  
> +int wake_up_process_interruptible(struct task_struct *p)
> +{
> +	return try_to_wake_up(p, TASK_INTERRUPTIBLE, 0);
> +}
> +EXPORT_SYMBOL_GPL(wake_up_process_interruptible);
> +
>  int wake_up_state(struct task_struct *p, unsigned int state)
>  {
>  	return try_to_wake_up(p, state, 0);

Re: [PATCH v9] ath9k: let sleep be interrupted when unregistering hwrng

[Jason A. Donenfeld]

Hi Eric,

On Tue, Jul 19, 2022 at 9:26 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
>
> "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:
> >
> > 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.
>
> Is there any chance you can name the new function
> wake_up_task_interruptible instead of wake_up_process_interruptible.
>
> The name wake_up_process is wrong now, it does not wake up all threads
> of a process.  The name dates back to before linux supported multiple
> threads in a process, so it is grandfathered in until someone gets
> changes it.  But please let's not have a new function with a incorrect
> and confusing name.

No problem. v+1 incoming.

Jason

[PATCH v10] ath9k: let sleep be interrupted when unregistering hwrng

[Jason A. Donenfeld]

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.

Cc: Kalle Valo <kvalo@kernel.org>
Cc: Rui Salvaterra <rsalvaterra@gmail.com>
Cc: Eric W. Biederman <ebiederm@xmission.com>
Cc: Valentin Schneider <vschneid@redhat.com>
Cc: stable@vger.kernel.org
Reported-by: Gregory Erwin <gregerwin256@gmail.com>
Tested-by: Gregory Erwin <gregerwin256@gmail.com>
Acked-by: Toke Høiland-Jørgensen <toke@toke.dk>
Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
Fixes: fcd09c90c3c5 ("ath9k: use hw_random API instead of directly dumping into random.c")
Link: https://lore.kernel.org/all/CAO+Okf6ZJC5-nTE_EJUGQtd8JiCkiEHytGgDsFGTEjs0c00giw@mail.gmail.com/
Link: https://lore.kernel.org/lkml/CAO+Okf5k+C+SE6pMVfPf-d8MfVPVq4PO7EY8Hys_DVXtent3HA@mail.gmail.com/
Link: https://bugs.archlinux.org/task/75138
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
Changes v9->v10:
- Call it wake_up_task_interruptible, per Eric's remark.
Changes v8->v9:
- Use EXPORT_SYMBOL_GPL instead of EXPORT_SYMBOL.
- Don't export wake_up_state, but rather have __set_notify_signal use
  wake_up_process_interruptible.

 drivers/char/hw_random/core.c        | 30 ++++++++++++++++++++++++----
 drivers/net/wireless/ath/ath9k/rng.c | 19 +++++++-----------
 include/linux/sched.h                |  1 +
 include/linux/sched/signal.h         |  2 +-
 kernel/sched/core.c                  |  6 ++++++
 5 files changed, 41 insertions(+), 17 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(&current_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(&current_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(&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);
 }
 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/include/linux/sched.h b/include/linux/sched.h
index c46f3a63b758..f164098fb614 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1936,6 +1936,7 @@ extern struct task_struct *find_get_task_by_vpid(pid_t nr);
 
 extern int wake_up_state(struct task_struct *tsk, unsigned int state);
 extern int wake_up_process(struct task_struct *tsk);
+extern int wake_up_task_interruptible(struct task_struct *tsk);
 extern void wake_up_new_task(struct task_struct *tsk);
 
 #ifdef CONFIG_SMP
diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
index cafbe03eed01..56a15f35e7b3 100644
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -364,7 +364,7 @@ static inline void clear_notify_signal(void)
 static inline bool __set_notify_signal(struct task_struct *task)
 {
 	return !test_and_set_tsk_thread_flag(task, TIF_NOTIFY_SIGNAL) &&
-	       !wake_up_state(task, TASK_INTERRUPTIBLE);
+	       !wake_up_task_interruptible(task);
 }
 
 /*
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index da0bf6fe9ecd..b178940185d7 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -4280,6 +4280,12 @@ int wake_up_process(struct task_struct *p)
 }
 EXPORT_SYMBOL(wake_up_process);
 
+int wake_up_task_interruptible(struct task_struct *p)
+{
+	return try_to_wake_up(p, TASK_INTERRUPTIBLE, 0);
+}
+EXPORT_SYMBOL_GPL(wake_up_task_interruptible);
+
 int wake_up_state(struct task_struct *p, unsigned int state)
 {
 	return try_to_wake_up(p, state, 0);
-- 
2.35.1

Re: [PATCH v10] ath9k: let sleep be interrupted when unregistering hwrng

[Eric W. Biederman]

"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:
>
> 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.

Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>

The fix as it is seems fine.

The whole design seems very strange to me.  I would think instead of
having a current hardware random number generator the kernel would
pull from every hardware random generator available.  Further that
we can get a userspace read all of the way into driver code for
a hardware random generator seems weird.    I would think in
practice we would want all of this filtered through /dev/random,
/dev/urandom, and the get_entropy syscall.

Which is a long way of saying it seems very strange to me that we
actually want to do what the code is doing.

Eric


> Cc: Kalle Valo <kvalo@kernel.org>
> Cc: Rui Salvaterra <rsalvaterra@gmail.com>
> Cc: Eric W. Biederman <ebiederm@xmission.com>
> Cc: Valentin Schneider <vschneid@redhat.com>
> Cc: stable@vger.kernel.org
> Reported-by: Gregory Erwin <gregerwin256@gmail.com>
> Tested-by: Gregory Erwin <gregerwin256@gmail.com>
> Acked-by: Toke Høiland-Jørgensen <toke@toke.dk>
> Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
> Fixes: fcd09c90c3c5 ("ath9k: use hw_random API instead of directly dumping into random.c")
> Link: https://lore.kernel.org/all/CAO+Okf6ZJC5-nTE_EJUGQtd8JiCkiEHytGgDsFGTEjs0c00giw@mail.gmail.com/
> Link: https://lore.kernel.org/lkml/CAO+Okf5k+C+SE6pMVfPf-d8MfVPVq4PO7EY8Hys_DVXtent3HA@mail.gmail.com/
> Link: https://bugs.archlinux.org/task/75138
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
> Changes v9->v10:
> - Call it wake_up_task_interruptible, per Eric's remark.
> Changes v8->v9:
> - Use EXPORT_SYMBOL_GPL instead of EXPORT_SYMBOL.
> - Don't export wake_up_state, but rather have __set_notify_signal use
>   wake_up_process_interruptible.
>
>  drivers/char/hw_random/core.c        | 30 ++++++++++++++++++++++++----
>  drivers/net/wireless/ath/ath9k/rng.c | 19 +++++++-----------
>  include/linux/sched.h                |  1 +
>  include/linux/sched/signal.h         |  2 +-
>  kernel/sched/core.c                  |  6 ++++++
>  5 files changed, 41 insertions(+), 17 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(&current_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(&current_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(&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);
>  }
>  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/include/linux/sched.h b/include/linux/sched.h
> index c46f3a63b758..f164098fb614 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1936,6 +1936,7 @@ extern struct task_struct *find_get_task_by_vpid(pid_t nr);
>  
>  extern int wake_up_state(struct task_struct *tsk, unsigned int state);
>  extern int wake_up_process(struct task_struct *tsk);
> +extern int wake_up_task_interruptible(struct task_struct *tsk);
>  extern void wake_up_new_task(struct task_struct *tsk);
>  
>  #ifdef CONFIG_SMP
> diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
> index cafbe03eed01..56a15f35e7b3 100644
> --- a/include/linux/sched/signal.h
> +++ b/include/linux/sched/signal.h
> @@ -364,7 +364,7 @@ static inline void clear_notify_signal(void)
>  static inline bool __set_notify_signal(struct task_struct *task)
>  {
>  	return !test_and_set_tsk_thread_flag(task, TIF_NOTIFY_SIGNAL) &&
> -	       !wake_up_state(task, TASK_INTERRUPTIBLE);
> +	       !wake_up_task_interruptible(task);
>  }
>  
>  /*
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index da0bf6fe9ecd..b178940185d7 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -4280,6 +4280,12 @@ int wake_up_process(struct task_struct *p)
>  }
>  EXPORT_SYMBOL(wake_up_process);
>  
> +int wake_up_task_interruptible(struct task_struct *p)
> +{
> +	return try_to_wake_up(p, TASK_INTERRUPTIBLE, 0);
> +}
> +EXPORT_SYMBOL_GPL(wake_up_task_interruptible);
> +
>  int wake_up_state(struct task_struct *p, unsigned int state)
>  {
>  	return try_to_wake_up(p, state, 0);

Re: [PATCH v10] ath9k: let sleep be interrupted when unregistering hwrng

[Jason A. Donenfeld]

Hi Eric,

On Tue, Jul 19, 2022 at 10:51 PM Eric W. Biederman
<ebiederm@xmission.com> wrote:
> Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>

Thank you.

> The whole design seems very strange to me.  I would think instead of
> having a current hardware random number generator the kernel would
> pull from every hardware random generator available.  Further that
> we can get a userspace read all of the way into driver code for
> a hardware random generator seems weird.    I would think in
> practice we would want all of this filtered through /dev/random,
> /dev/urandom, and the get_entropy syscall.

Yes indeed. In general, the hwrng interface is kind of badly designed
and a bit of a nuisance. I've spent the last few months reworking
random.c and that's finally nearing okay enough shape. Possibly after
I'll turn my attention to a real overhaul of hwrng too (assuming
Herbert gives me lattitude to do that, I guess; I don't maintain that
code). The main goal anyhow ought be to just non-invasively shephard
bits into random.c, with additional frills being merely additional.

Jason

Re: [PATCH v10] ath9k: let sleep be interrupted when unregistering hwrng

[Valentin Schneider]

On 19/07/22 22:11, Jason A. Donenfeld wrote:

> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index c46f3a63b758..f164098fb614 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1936,6 +1936,7 @@ extern struct task_struct *find_get_task_by_vpid(pid_t nr);
>
>  extern int wake_up_state(struct task_struct *tsk, unsigned int state);
>  extern int wake_up_process(struct task_struct *tsk);
> +extern int wake_up_task_interruptible(struct task_struct *tsk);
>  extern void wake_up_new_task(struct task_struct *tsk);
>
>  #ifdef CONFIG_SMP
> diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
> index cafbe03eed01..56a15f35e7b3 100644
> --- a/include/linux/sched/signal.h
> +++ b/include/linux/sched/signal.h
> @@ -364,7 +364,7 @@ static inline void clear_notify_signal(void)
>  static inline bool __set_notify_signal(struct task_struct *task)
>  {
>       return !test_and_set_tsk_thread_flag(task, TIF_NOTIFY_SIGNAL) &&
> -	       !wake_up_state(task, TASK_INTERRUPTIBLE);
> +	       !wake_up_task_interruptible(task);
>  }
>
>  /*
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index da0bf6fe9ecd..b178940185d7 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -4280,6 +4280,12 @@ int wake_up_process(struct task_struct *p)
>  }
>  EXPORT_SYMBOL(wake_up_process);
>
> +int wake_up_task_interruptible(struct task_struct *p)
> +{
> +	return try_to_wake_up(p, TASK_INTERRUPTIBLE, 0);
> +}
> +EXPORT_SYMBOL_GPL(wake_up_task_interruptible);
> +
>  int wake_up_state(struct task_struct *p, unsigned int state)
>  {
>       return try_to_wake_up(p, state, 0);
> --
> 2.35.1


The sched changes are unfortunate, as I had understood it the alternative
would be fixing all sleeping hwrng's to make them have a proper wait
pattern that doesn't require being sent a signal to avoid missing events,
i.e. instead of 

  hwrng->read():                               devm_hwrng_unregister():
    schedule_timeout_interruptible(x);           set_notify_signal(waiting_reader);

do

  hwrng->read():                               devm_hwrng_unregister():
    set_current_state(TASK_INTERRUPTIBLE)        rng->dying = true;
    if (!rng->dying)                             wake_up_process(waiting_reader);
        schedule_timeout(x);

I had initially convinced myself this would be somewhat involved, but
writing the above I thought maybe not... The below is applied on top of
your v10, would you be able to test whether it actually works?

I apologize for telling you to do one thing and then suggesting something else...

IMO set_notify_signal() is for interrupting tasks that are in a wait-loop
that has nothing to do with the calling code (e.g. task_work, I assume
livepatching does that for the same reason), but here it's hwrng core code
interrupting a sleeping hwrng device.

It does however mean patching up any sleeping hwrng (a quick search tells
me there are more, e.g. npcm-rng does readb_poll_timeout())

---

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index df45c265878e..40a73490bfdc 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -522,9 +522,12 @@ static int hwrng_fillfn(void *unused)
 			break;
 
 		if (rc <= 0) {
-			if (kthread_should_stop())
+			set_current_state(TASK_INTERRUPTIBLE);
+			if (kthread_should_stop()) {
+				__set_current_state(TASK_RUNNING);
 				break;
-			schedule_timeout_interruptible(HZ * 10);
+			}
+			schedule_timeout(HZ * 10);
 			continue;
 		}
 
@@ -581,6 +584,8 @@ int hwrng_register(struct hwrng *rng)
 	init_completion(&rng->cleanup_done);
 	complete(&rng->cleanup_done);
 
+	rng->unregistering = false;
+
 	if (!current_rng ||
 	    (!cur_rng_set_by_user && rng->quality > current_rng->quality)) {
 		/*
@@ -630,8 +635,10 @@ void hwrng_unregister(struct hwrng *rng)
 
 	rcu_read_lock();
 	waiting_reader = xchg(&current_waiting_reader, UNREGISTERING_READER);
-	if (waiting_reader && waiting_reader != UNREGISTERING_READER)
-		set_notify_signal(waiting_reader);
+	if (waiting_reader && waiting_reader != UNREGISTERING_READER) {
+		rng->unregistering = true;
+		wake_up_process(waiting_reader);
+	}
 	rcu_read_unlock();
 	old_rng = current_rng;
 	list_del(&rng->list);
diff --git a/drivers/net/wireless/ath/ath9k/rng.c b/drivers/net/wireless/ath/ath9k/rng.c
index 8980dc36509e..35cac38054b5 100644
--- a/drivers/net/wireless/ath/ath9k/rng.c
+++ b/drivers/net/wireless/ath/ath9k/rng.c
@@ -75,9 +75,17 @@ 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 ||
-		    ((current->flags & PF_KTHREAD) && kthread_should_stop()) ||
-		    schedule_timeout_interruptible(ath9k_rng_delay_get(++fail_stats)))
+		if (!wait || !max || likely(bytes_read) || fail_stats > 110)
+			break;
+
+		set_current_state(TASK_INTERRUPTIBLE);
+		if (rng->unregistering ||
+		    ((current->flags & PF_KTHREAD) && kthread_should_stop())) {
+			__set_current_state(TASK_RUNNING);
+			break;
+		}
+
+		if (schedule_timeout(ath9k_rng_delay_get(++fail_stats)))
 			break;
 	}
 
diff --git a/include/linux/hw_random.h b/include/linux/hw_random.h
index aa1d4da03538..778f10dfa12b 100644
--- a/include/linux/hw_random.h
+++ b/include/linux/hw_random.h
@@ -45,6 +45,7 @@ struct hwrng {
 	int (*read)(struct hwrng *rng, void *data, size_t max, bool wait);
 	unsigned long priv;
 	unsigned short quality;
+	int unregistering;
 
 	/* internal. */
 	struct list_head list;
diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
index 56a15f35e7b3..9b94a8f18b04 100644
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -364,7 +364,7 @@ static inline void clear_notify_signal(void)
 static inline bool __set_notify_signal(struct task_struct *task)
 {
 	return !test_and_set_tsk_thread_flag(task, TIF_NOTIFY_SIGNAL) &&
-	       !wake_up_task_interruptible(task);
+		!wake_up_state(task, TASK_INTERRUPTIBLE);
 }
 
 /*
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index e3e675eef63d..a463dbc92fcd 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -4279,12 +4279,6 @@ int wake_up_process(struct task_struct *p)
 }
 EXPORT_SYMBOL(wake_up_process);
 
-int wake_up_task_interruptible(struct task_struct *p)
-{
-	return try_to_wake_up(p, TASK_INTERRUPTIBLE, 0);
-}
-EXPORT_SYMBOL_GPL(wake_up_task_interruptible);
-
 int wake_up_state(struct task_struct *p, unsigned int state)
 {
 	return try_to_wake_up(p, state, 0);

Re: [PATCH v10] ath9k: let sleep be interrupted when unregistering hwrng

[Jason A. Donenfeld]

Hi Valentin,

On Fri, Jul 22, 2022 at 10:09 PM Valentin Schneider <vschneid@redhat.com> wrote:
> I had initially convinced myself this would be somewhat involved, but
> writing the above I thought maybe not... The below is applied on top of
> your v10, would you be able to test whether it actually works?
> It does however mean patching up any sleeping hwrng (a quick search tells
> me there are more, e.g. npcm-rng does readb_poll_timeout())

I'm not able to test this easily, no (I don't own any hardware), and
I'm not going to put in the effort to rewrite/audit every sleeping
hwrng. That's not a good use of time, given the numerous other
problems the framework has (briefly discussed with Eric). Instead,
maybe at some point I'll look into overhauling all of this so that
none of this will be required anyway. So I think v10 is my final
submission on this.

But if you'd like to attempt more comprehensive changes throughout the
tree on all the drivers and do something large, I guess you can do
that independently (since you mentioned your thing works on top of
v10). And this way v10 still exists to fix the actual bug that's
currently reeking havoc. On the other hand, maybe don't bother, and we
can look into fixing the whole rats nest properly in some months when
I'm more motivated to jump into hwrng.

Jason

Re: [PATCH v10] ath9k: let sleep be interrupted when unregistering hwrng

[Valentin Schneider]

Hi Jason,

On 22/07/22 22:13, Jason A. Donenfeld wrote:
> Hi Valentin,
>
> On Fri, Jul 22, 2022 at 10:09 PM Valentin Schneider <vschneid@redhat.com> wrote:
>> I had initially convinced myself this would be somewhat involved, but
>> writing the above I thought maybe not... The below is applied on top of
>> your v10, would you be able to test whether it actually works?
>> It does however mean patching up any sleeping hwrng (a quick search tells
>> me there are more, e.g. npcm-rng does readb_poll_timeout())
>
> I'm not able to test this easily, no (I don't own any hardware), and
> I'm not going to put in the effort to rewrite/audit every sleeping
> hwrng. That's not a good use of time, given the numerous other
> problems the framework has (briefly discussed with Eric). Instead,
> maybe at some point I'll look into overhauling all of this so that
> none of this will be required anyway. So I think v10 is my final
> submission on this.
>

I think that's fair, I hope I didn't discourage you too much from
contributing in that area.

> But if you'd like to attempt more comprehensive changes throughout the
> tree on all the drivers and do something large, I guess you can do
> that independently (since you mentioned your thing works on top of
> v10). And this way v10 still exists to fix the actual bug that's
> currently reeking havoc. On the other hand, maybe don't bother, and we
> can look into fixing the whole rats nest properly in some months when
> I'm more motivated to jump into hwrng.
>

Just to make sure I'm on the same page as you - is your patch solely for
ath9k, or is it supposed to fix other drivers?

AFAICT your changes to hw_random/core.c work with any hwnrg driver that
does a variant of schedule_timeout() during rng_get_data(), whereas what I
suggested only works for "opted-in" drivers (just ath9k ATM), but it
doesn't break the other drivers in any way.

So if ath9k is widely used / a problem for lots of folks, we could just fix
that one and leave the others TBD. What do you think?

Re: [PATCH v10] ath9k: let sleep be interrupted when unregistering hwrng

[Jason A. Donenfeld]

Hi Valentin,

On Mon, Jul 25, 2022 at 12:09 PM Valentin Schneider <vschneid@redhat.com> wrote:
> > maybe at some point I'll look into overhauling all of this so that
> > none of this will be required anyway. So I think v10 is my final
> > submission on this.
>
> I think that's fair, I hope I didn't discourage you too much from
> contributing in that area.

While not strictly necessary because of Eric's ack, since you continue
to grow this thread that addresses an active bug people are suffering
from, it might be some very useful signaling if you too would provide
your Acked-by, so that Kalle picks this up and people's laptops work
again.

>
> Just to make sure I'm on the same page as you - is your patch solely for
> ath9k, or is it supposed to fix other drivers?

The intent here is ath9k, but in the process it of course fixes other
things too, and I'm quite pleased with the multiple-birds-one-stone
consequence.

> So if ath9k is widely used / a problem for lots of folks, we could just fix
> that one and leave the others TBD. What do you think?

No, that kind of band aid doesn't really sit well, especially as
you've proposed struct changes inside hwrng. I'm not going to do that.

As mentioned, v10 is my final submission here. If you'd like to Ack
it, please go ahead. If not, and if as a consequence Kalle doesn't
pick up the patch, perhaps you'll develop this yourself moving forward
and you'll also fix the longstanding problems with hwrng so that I
don't have to. In case it's not clear to you: I'm no longer interested
in any aspect of this discussion, I find the current patch
satisfactory of numerous goals, and I would appreciate this whole
thing being over. I am no longer available to work further on this
patch.

Thanks,
Jason

Re: [PATCH v10] ath9k: let sleep be interrupted when unregistering hwrng

[Valentin Schneider]

On 25/07/22 13:41, Jason A. Donenfeld wrote:
> Hi Valentin,
>
> On Mon, Jul 25, 2022 at 12:09 PM Valentin Schneider <vschneid@redhat.com> wrote:
>> > maybe at some point I'll look into overhauling all of this so that
>> > none of this will be required anyway. So I think v10 is my final
>> > submission on this.
>>
>> I think that's fair, I hope I didn't discourage you too much from
>> contributing in that area.
>
> While not strictly necessary because of Eric's ack, since you continue
> to grow this thread that addresses an active bug people are suffering
> from, it might be some very useful signaling if you too would provide
> your Acked-by, so that Kalle picks this up and people's laptops work
> again.
>

I don't think the __set_notify_signal() approach is functionally wrong, but
I also believe it isn't the proper tool for the job (for reasons I wrote
previously).

I won't ack it, but I won't nack it either if others find it satisfactory.