2017-06-22 01:24:57

by Miaoqing Pan

[permalink] [raw]
Subject: [PATCH 1/3] ath9k: fixed an invalid pointer dereference

From: Miaoqing Pan <[email protected]>

The bug was triggered when do suspend/resuming continuously
on Dell XPS L322X/0PJHXN version 9333 (2013) with kernel
4.12.0-041200rc4-generic. But can't reproduce on DELL
E5440 + AR9300 PCIE chips.

The warning is caused by accessing invalid pointer sc->rng_task.
sc->rng_task is not be cleared after kthread_stop(sc->rng_task)
be called in ath9k_rng_stop(). Because the kthread is stopped
before ath9k_rng_kthread() be scheduled.

So set sc->rng_task to null after kthread_stop(sc->rng_task) to
resolve this issue.

WARNING: CPU: 0 PID: 984 at linux/kernel/kthread.c:71 kthread_stop+0xf1/0x100
CPU: 0 PID: 984 Comm: NetworkManager Not tainted 4.12.0-041200rc4-generic #201706042031
Hardware name: Dell Inc. Dell System XPS L322X/0PJHXN, BIOS A09 05/15/2013
task: ffff950170fdda00 task.stack: ffffa22c01538000
RIP: 0010:kthread_stop+0xf1/0x100
RSP: 0018:ffffa22c0153b5b0 EFLAGS: 00010246
RAX: ffffffffa6257800 RBX: ffff950171b79560 RCX: 0000000000000000
RDX: 0000000080000000 RSI: 000000007fffffff RDI: ffff9500ac9a9680
RBP: ffffa22c0153b5c8 R08: 0000000000000000 R09: 0000000000000000
R10: ffffa22c0153b648 R11: ffff9501768004b8 R12: ffff9500ac9a9680
R13: ffff950171b79f70 R14: ffff950171b78780 R15: ffff9501749dc018
FS: 00007f0d6bfd5540(0000) GS:ffff95017f200000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007fc190161a08 CR3: 0000000232906000 CR4: 00000000001406f0
Call Trace:
ath9k_rng_stop+0x1a/0x20 [ath9k]
ath9k_stop+0x3b/0x1d0 [ath9k]
drv_stop+0x33/0xf0 [mac80211]
ieee80211_stop_device+0x43/0x50 [mac80211]
ieee80211_do_stop+0x4f2/0x810 [mac80211]

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=196043
Reported-by: Giulio Genovese <[email protected]>
Tested-by: Giulio Genovese <[email protected]>
Signed-off-by: Miaoqing Pan <[email protected]>
---
drivers/net/wireless/ath/ath9k/rng.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath9k/rng.c b/drivers/net/wireless/ath/ath9k/rng.c
index 568b1c6..73f46fb 100644
--- a/drivers/net/wireless/ath/ath9k/rng.c
+++ b/drivers/net/wireless/ath/ath9k/rng.c
@@ -120,6 +120,8 @@ void ath9k_rng_start(struct ath_softc *sc)

void ath9k_rng_stop(struct ath_softc *sc)
{
- if (sc->rng_task)
+ if (sc->rng_task) {
kthread_stop(sc->rng_task);
+ sc->rng_task = NULL;
+ }
}
--
1.9.1


2017-06-28 16:55:14

by Kalle Valo

[permalink] [raw]
Subject: Re: [1/3] ath9k: fixed an invalid pointer dereference

miaoqing pan <[email protected]> wrote:

> The bug was triggered when do suspend/resuming continuously
> on Dell XPS L322X/0PJHXN version 9333 (2013) with kernel
> 4.12.0-041200rc4-generic. But can't reproduce on DELL
> E5440 + AR9300 PCIE chips.
>
> The warning is caused by accessing invalid pointer sc->rng_task.
> sc->rng_task is not be cleared after kthread_stop(sc->rng_task)
> be called in ath9k_rng_stop(). Because the kthread is stopped
> before ath9k_rng_kthread() be scheduled.
>
> So set sc->rng_task to null after kthread_stop(sc->rng_task) to
> resolve this issue.
>
> WARNING: CPU: 0 PID: 984 at linux/kernel/kthread.c:71 kthread_stop+0xf1/0x100
> CPU: 0 PID: 984 Comm: NetworkManager Not tainted 4.12.0-041200rc4-generic #201706042031
> Hardware name: Dell Inc. Dell System XPS L322X/0PJHXN, BIOS A09 05/15/2013
> task: ffff950170fdda00 task.stack: ffffa22c01538000
> RIP: 0010:kthread_stop+0xf1/0x100
> RSP: 0018:ffffa22c0153b5b0 EFLAGS: 00010246
> RAX: ffffffffa6257800 RBX: ffff950171b79560 RCX: 0000000000000000
> RDX: 0000000080000000 RSI: 000000007fffffff RDI: ffff9500ac9a9680
> RBP: ffffa22c0153b5c8 R08: 0000000000000000 R09: 0000000000000000
> R10: ffffa22c0153b648 R11: ffff9501768004b8 R12: ffff9500ac9a9680
> R13: ffff950171b79f70 R14: ffff950171b78780 R15: ffff9501749dc018
> FS: 00007f0d6bfd5540(0000) GS:ffff95017f200000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007fc190161a08 CR3: 0000000232906000 CR4: 00000000001406f0
> Call Trace:
> ath9k_rng_stop+0x1a/0x20 [ath9k]
> ath9k_stop+0x3b/0x1d0 [ath9k]
> drv_stop+0x33/0xf0 [mac80211]
> ieee80211_stop_device+0x43/0x50 [mac80211]
> ieee80211_do_stop+0x4f2/0x810 [mac80211]
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=196043
> Reported-by: Giulio Genovese <[email protected]>
> Tested-by: Giulio Genovese <[email protected]>
> Cc: <[email protected]>
> Signed-off-by: Miaoqing Pan <[email protected]>
> Signed-off-by: Kalle Valo <[email protected]>

3 patches applied to ath-next branch of ath.git, thanks.

07246c115801 ath9k: fix an invalid pointer dereference in ath9k_rng_stop()
473becac4b31 ath9k: avoid potential freezing during random generator read
f23cdfb3fe8f ath9k: Use mutex_lock to avoid potential race in start/stop rng

--
https://patchwork.kernel.org/patch/9803211/

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

2017-06-28 16:50:10

by Kalle Valo

[permalink] [raw]
Subject: Re: [1/3] ath9k: fixed an invalid pointer dereference

miaoqing pan <[email protected]> wrote:

> The bug was triggered when do suspend/resuming continuously
> on Dell XPS L322X/0PJHXN version 9333 (2013) with kernel
> 4.12.0-041200rc4-generic. But can't reproduce on DELL
> E5440 + AR9300 PCIE chips.
>
> The warning is caused by accessing invalid pointer sc->rng_task.
> sc->rng_task is not be cleared after kthread_stop(sc->rng_task)
> be called in ath9k_rng_stop(). Because the kthread is stopped
> before ath9k_rng_kthread() be scheduled.
>
> So set sc->rng_task to null after kthread_stop(sc->rng_task) to
> resolve this issue.
>
> WARNING: CPU: 0 PID: 984 at linux/kernel/kthread.c:71 kthread_stop+0xf1/0x100
> CPU: 0 PID: 984 Comm: NetworkManager Not tainted 4.12.0-041200rc4-generic #201706042031
> Hardware name: Dell Inc. Dell System XPS L322X/0PJHXN, BIOS A09 05/15/2013
> task: ffff950170fdda00 task.stack: ffffa22c01538000
> RIP: 0010:kthread_stop+0xf1/0x100
> RSP: 0018:ffffa22c0153b5b0 EFLAGS: 00010246
> RAX: ffffffffa6257800 RBX: ffff950171b79560 RCX: 0000000000000000
> RDX: 0000000080000000 RSI: 000000007fffffff RDI: ffff9500ac9a9680
> RBP: ffffa22c0153b5c8 R08: 0000000000000000 R09: 0000000000000000
> R10: ffffa22c0153b648 R11: ffff9501768004b8 R12: ffff9500ac9a9680
> R13: ffff950171b79f70 R14: ffff950171b78780 R15: ffff9501749dc018
> FS: 00007f0d6bfd5540(0000) GS:ffff95017f200000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007fc190161a08 CR3: 0000000232906000 CR4: 00000000001406f0
> Call Trace:
> ath9k_rng_stop+0x1a/0x20 [ath9k]
> ath9k_stop+0x3b/0x1d0 [ath9k]
> drv_stop+0x33/0xf0 [mac80211]
> ieee80211_stop_device+0x43/0x50 [mac80211]
> ieee80211_do_stop+0x4f2/0x810 [mac80211]
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=196043
> Reported-by: Giulio Genovese <[email protected]>
> Tested-by: Giulio Genovese <[email protected]>
> Cc: <[email protected]>
> Signed-off-by: Miaoqing Pan <[email protected]>
> Signed-off-by: Kalle Valo <[email protected]>

I Cced stable and improved the title a bit.

--
https://patchwork.kernel.org/patch/9803211/

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

2017-06-22 01:25:07

by Miaoqing Pan

[permalink] [raw]
Subject: [PATCH 3/3] ath9k: Use mutex_lock to avoid potential race in start/stop rng

From: Miaoqing Pan <[email protected]>

Move ath9k_rng_stop/ath9k_rng_start pair into critical section,
use mutex_lock to void potential race accessing.

Signed-off-by: Miaoqing Pan <[email protected]>
---
drivers/net/wireless/ath/ath9k/main.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index 9e65d14..8b4ac7f 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -731,12 +731,12 @@ static int ath9k_start(struct ieee80211_hw *hw)

spin_unlock_bh(&sc->sc_pcu_lock);

+ ath9k_rng_start(sc);
+
mutex_unlock(&sc->mutex);

ath9k_ps_restore(sc);

- ath9k_rng_start(sc);
-
return 0;
}

@@ -826,10 +826,10 @@ static void ath9k_stop(struct ieee80211_hw *hw)

ath9k_deinit_channel_context(sc);

- ath9k_rng_stop(sc);
-
mutex_lock(&sc->mutex);

+ ath9k_rng_stop(sc);
+
ath_cancel_work(sc);

if (test_bit(ATH_OP_INVALID, &common->op_flags)) {
--
1.9.1

2017-06-22 01:25:02

by Miaoqing Pan

[permalink] [raw]
Subject: [PATCH 2/3] ath9k: avoid potential freezing

From: Miaoqing Pan <[email protected]>

In the worst case, ath9k_rng_stop() may take 10s to stop rng kthread.
The time is too long for users, use wait_event_interruptible_timeout()
instead of msleep_interruptible(), wakup immediately once
kthread_should_stop() is true.

Signed-off-by: Miaoqing Pan <[email protected]>
---
drivers/net/wireless/ath/ath9k/rng.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath9k/rng.c b/drivers/net/wireless/ath/ath9k/rng.c
index 73f46fb..f9d3d6e 100644
--- a/drivers/net/wireless/ath/ath9k/rng.c
+++ b/drivers/net/wireless/ath/ath9k/rng.c
@@ -24,6 +24,8 @@
#define ATH9K_RNG_BUF_SIZE 320
#define ATH9K_RNG_ENTROPY(x) (((x) * 8 * 10) >> 5) /* quality: 10/32 */

+static DECLARE_WAIT_QUEUE_HEAD(rng_queue);
+
static int ath9k_rng_data_read(struct ath_softc *sc, u32 *buf, u32 buf_size)
{
int i, j;
@@ -85,7 +87,9 @@ static int ath9k_rng_kthread(void *data)
ATH9K_RNG_BUF_SIZE);
if (unlikely(!bytes_read)) {
delay = ath9k_rng_delay_get(++fail_stats);
- msleep_interruptible(delay);
+ wait_event_interruptible_timeout(rng_queue,
+ kthread_should_stop(),
+ msecs_to_jiffies(delay));
continue;
}

--
1.9.1