OOPS in ath5k when setting coverage class

OOPS in ath5k when setting coverage class

[Steve Brown]

 From the printk's, it appears that ah->ah_current_channel isn't set at 
the time of the callback.

The following produces the oops with compat-wireless-2010-06-10.

Steve

---------------------------------
rmmod ath5k
insmod ath5k
iw phy0 set distance 11000

pcu.c:ath5k_hw_set_coverage_class:849 ah:cf8c0000 coverage_class:25
pcu.c:ath5k_hw_get_default_slottime:254 ah:cf8c0000 channel:(null)
BUG: unable to handle kernel NULL pointer dereference at 00000006
IP: [<d0a1ff24>] ath5k_hw_set_coverage_class+0x74/0x1b0 [ath5k]
*pde = 00000000
Oops: 0000 [#1]
last sysfs file: /sys/devices/pci0000:00/0000:00:0e.0/ieee80211/phy0/index
Modules linked in: usbhid option usb_storage usbserial usblp evdev lm90 
scx200_acb i2c_algo_bit i2c_dev i2c_core via_rhine ohci_hcd ne2k_pci 
8390 leds_alix2 xt_IMQ imq nf_nat_tftp nf_conntrack_tftp nf_nat_irc nf_cc

Pid: 1597, comm: iw Not tainted (2.6.32.14 #8)
EIP: 0060:[<d0a1ff24>] EFLAGS: 00010296 CPU: 0
EIP is at ath5k_hw_set_coverage_class+0x74/0x1b0 [ath5k]
EAX: 000000c2 EBX: 00000000 ECX: ffffffff EDX: c12d2080
ESI: 00000019 EDI: cf8c0000 EBP: d0a30edc ESP: cfa09bf4
  DS: 007b ES: 007b FS: 0000 GS: 0000 SS: 0068
Process iw (pid: 1597, ti=cfa09000 task=cf88a000 task.ti=cfa09000)
Stack:
  d0a34f35 d0a353f8 d0a30edc 000000fe cf8c0000 00000000 1900063d cfa8c9e0
<0> cfa8c9e8 cfa8c0c0 cfa8c000 d0a27f0c 199d84b4 cfa8c200 00000010 d09bfdc7
<0> 00000000 00000000 ffffffff d08e0d28 cf9263c0 00000001 cfa09cc4 00000000
Call Trace:
  [<d0a27f0c>] ? ath5k_hw_attach+0xc8c/0x3c10 [ath5k]
  [<d09bfdc7>] ? __ieee80211_request_smps+0x1347/0x1580 [mac80211]
  [<d08e0d28>] ? nl80211_send_scan_start+0x7b8/0x4520 [cfg80211]
  [<c10f5db9>] ? nla_parse+0x59/0xc0
  [<c11ca8d9>] ? genl_rcv_msg+0x169/0x1a0
  [<c11ca770>] ? genl_rcv_msg+0x0/0x1a0
  [<c11c7e68>] ? netlink_rcv_skb+0x38/0x90
  [<c11c9649>] ? genl_rcv+0x19/0x30
  [<c11c7c03>] ? netlink_unicast+0x1b3/0x220
  [<c11c893e>] ? netlink_sendmsg+0x26e/0x290
  [<c11a409e>] ? sock_sendmsg+0xbe/0xf0
  [<c1032780>] ? autoremove_wake_function+0x0/0x50
  [<c104d846>] ? __alloc_pages_nodemask+0x106/0x530
  [<c1074933>] ? do_lookup+0x53/0x1b0
  [<c10766f9>] ? __link_path_walk+0x9b9/0x9e0
  [<c11acab0>] ? verify_iovec+0x50/0x90
  [<c11a42b1>] ? sys_sendmsg+0x1e1/0x270
  [<c1048e50>] ? find_get_page+0x10/0x50
  [<c104a96f>] ? filemap_fault+0x5f/0x370
  [<c1059159>] ? __do_fault+0x319/0x370
  [<c11a55b4>] ? sys_socketcall+0x244/0x290
  [<c101962c>] ? do_page_fault+0x1ec/0x270
  [<c1019440>] ? do_page_fault+0x0/0x270
  [<c1002ae5>] ? syscall_call+0x7/0xb
Code: 00 b8 fe 00 00 00 b9 f8 53 a3 d0 89 5c 24 14 89 7c 24 10 89 44 24 
0c 89 6c 24 08 89 4c 24 04 c7 04 24 35 4f a3 d0 e8 7c 30 60 f0 <0f> b7 
43 06 ba 06 00 00 00 a8 10 75 0e 83 e0 20 83 f8 01 19 d2
EIP: [<d0a1ff24>] ath5k_hw_set_coverage_class+0x74/0x1b0 [ath5k] SS:ESP 
0068:cfa09bf4
CR2: 0000000000000006
---[ end trace 54f73d6b10ceb87b ]---
Killed

Re: OOPS in ath5k when setting coverage class

[Bob Copeland]

On Wed, Jun 16, 2010 at 3:32 PM, Steve Brown <sbrown@cortland.com> wrote:
> From the printk's, it appears that ah->ah_current_channel isn't set at the
> time of the callback.
>
> The following produces the oops with compat-wireless-2010-06-10.

Thanks for the report - your analysis looks spot on.
I'll take a look tonight.

-- 
Bob Copeland %% www.bobcopeland.com

Re: OOPS in ath5k when setting coverage class

[Steve Brown]

On 06/17/2010 11:39 AM, Bob Copeland wrote:
> On Wed, Jun 16, 2010 at 3:32 PM, Steve Brown<sbrown@cortland.com>  wrote:
>    
>>  From the printk's, it appears that ah->ah_current_channel isn't set at the
>> time of the callback.
>>
>> The following produces the oops with compat-wireless-2010-06-10.
>>      
> Thanks for the report - your analysis looks spot on.
> I'll take a look tonight.
>
>    
I went a little further and tested the following patch.

If it makes sense to you, I'll submit it.

Steve

---

--- a/drivers/net/wireless/ath/ath5k/pcu.c.orig    2010-06-17 
10:41:29.070150186 -0600
+++ b/drivers/net/wireless/ath/ath5k/pcu.c    2010-06-17 
10:56:27.529275091 -0600
@@ -251,7 +251,6 @@
  static unsigned int ath5k_hw_get_default_slottime(struct ath5k_hw *ah)
  {
      struct ieee80211_channel *channel = ah->ah_current_channel;
-
      if (channel->hw_value & CHANNEL_TURBO)
          return 6; /* both turbo modes */

@@ -846,13 +845,23 @@
  void ath5k_hw_set_coverage_class(struct ath5k_hw *ah, u8 coverage_class)
  {
      /* As defined by IEEE 802.11-2007 17.3.8.6 */
-    int slot_time = ath5k_hw_get_default_slottime(ah) + 3 * coverage_class;
-    int ack_timeout = ath5k_hw_get_default_sifs(ah) + slot_time;
-    int cts_timeout = ack_timeout;
+    int slot_time, ack_timeout, cts_timeout;
+
+    ah->ah_coverage_class = coverage_class;

+    /*
+     * If channel isn't set, just exit.
+     * We will be called again on a channel change.
+     */
+    if (ah->ah_current_channel == NULL)
+        return;
+
+    slot_time = ath5k_hw_get_default_slottime(ah) + 3 * coverage_class;
      ath5k_hw_set_slot_time(ah, slot_time);
+
+    ack_timeout = ath5k_hw_get_default_sifs(ah) + slot_time;
      ath5k_hw_set_ack_timeout(ah, ack_timeout);
-    ath5k_hw_set_cts_timeout(ah, cts_timeout);

-    ah->ah_coverage_class = coverage_class;
+    cts_timeout = ack_timeout;
+    ath5k_hw_set_cts_timeout(ah, cts_timeout);
  }

Re: OOPS in ath5k when setting coverage class

[Bob Copeland]

On Thu, Jun 17, 2010 at 2:49 PM, Steve Brown <sbrown@cortland.com> wrote:
> I went a little further and tested the following patch.
>
> If it makes sense to you, I'll submit it.


Looks fine to me, but I do want to do a quick audit of the driver
to see if there are others.  We just fixed another one in setting
the antenna mode with the same root cause.

-- 
Bob Copeland %% www.bobcopeland.com

Re: OOPS in ath5k when setting coverage class

[Bob Copeland]

On Thu, Jun 17, 2010 at 12:49:51PM -0600, Steve Brown wrote:
> I went a little further and tested the following patch.
>
> If it makes sense to you, I'll submit it.

So I looked over the code and found a few more trouble spots.
What about something like this instead?  It shouldn't hurt to
use a default channel but could avoid another round or two of
whack-a-mole...

From: Bob Copeland <me@bobcopeland.com>
Date: Thu, 17 Jun 2010 23:05:55 -0400
Subject: [PATCH] ath5k: initialize ah->ah_current_channel

ath5k assumes ah_current_channel is always a valid pointer in
several places, but a newly created interface may not have a
channel.  To avoid null pointer dereferences, set it up to point
to the first available channel until later reconfigured.

Signed-off-by: Bob Copeland <me@bobcopeland.com>
---
 drivers/net/wireless/ath/ath5k/attach.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/ath/ath5k/attach.c b/drivers/net/wireless/ath/ath5k/attach.c
index ef2dc1d..b32e28c 100644
--- a/drivers/net/wireless/ath/ath5k/attach.c
+++ b/drivers/net/wireless/ath/ath5k/attach.c
@@ -126,6 +126,7 @@ int ath5k_hw_attach(struct ath5k_softc *sc)
 	ah->ah_ant_mode = AR5K_ANTMODE_DEFAULT;
 	ah->ah_noise_floor = -95;	/* until first NF calibration is run */
 	sc->ani_state.ani_mode = ATH5K_ANI_MODE_AUTO;
+	ah->ah_current_channel = &sc->channels[0];
 
 	/*
 	 * Find the mac version
-- 
1.6.3.3


-- 
Bob Copeland %% www.bobcopeland.com

Re: OOPS in ath5k when setting coverage class

[Steve Brown]

On 06/17/2010 09:16 PM, Bob Copeland wrote:
> On Thu, Jun 17, 2010 at 12:49:51PM -0600, Steve Brown wrote:
>    
>> I went a little further and tested the following patch.
>>
>> If it makes sense to you, I'll submit it.
>>      
> So I looked over the code and found a few more trouble spots.
> What about something like this instead?  It shouldn't hurt to
> use a default channel but could avoid another round or two of
> whack-a-mole...
>
> From: Bob Copeland<me@bobcopeland.com>
> Date: Thu, 17 Jun 2010 23:05:55 -0400
> Subject: [PATCH] ath5k: initialize ah->ah_current_channel
>
> ath5k assumes ah_current_channel is always a valid pointer in
> several places, but a newly created interface may not have a
> channel.  To avoid null pointer dereferences, set it up to point
> to the first available channel until later reconfigured.
>
> Signed-off-by: Bob Copeland<me@bobcopeland.com>
> ---
>   drivers/net/wireless/ath/ath5k/attach.c |    1 +
>   1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath5k/attach.c b/drivers/net/wireless/ath/ath5k/attach.c
> index ef2dc1d..b32e28c 100644
> --- a/drivers/net/wireless/ath/ath5k/attach.c
> +++ b/drivers/net/wireless/ath/ath5k/attach.c
> @@ -126,6 +126,7 @@ int ath5k_hw_attach(struct ath5k_softc *sc)
>   	ah->ah_ant_mode = AR5K_ANTMODE_DEFAULT;
>   	ah->ah_noise_floor = -95;	/* until first NF calibration is run */
>   	sc->ani_state.ani_mode = ATH5K_ANI_MODE_AUTO;
> +	ah->ah_current_channel =&sc->channels[0];
>
>   	/*
>   	 * Find the mac version
>    
Applied and tested. Works.

Thanks,

Steve