Please note: This project is no longer active. The website is kept online for historic purposes only.
If you´re looking for a Linux driver for your Atheros WLAN device, you should continue here .

Ticket #962 (new defect)

Opened 13 years ago

Last modified 12 years ago

[patch] stale keycache indexes causing usage of incorrect node pointers

Reported by: xmxwx@asn.pl Assigned to:
Priority: major Milestone: version 0.9.x - progressive release candidate phase
Component: madwifi: other Version: v0.9.2
Keywords: Cc:
Patch is attached: 1 Pending:

Description

This patch fixes a bug which caused incorrect node pointer to be passed from ath_rx_tasklet() to ieee80211_input() under some specific circumstances. This in turn caused subsequent actions (most notably - responses to management frames) to be executed in a wrong environment, so that STAs were sent frames not intented for them (and inversely, the intended adressees didn't get the frames at all).

The "specific circumstances" mentioned above are when:

  • a node is freed
  • its corresponding keycache entry (sc_keyixmap) is destroyed (ath_hal_keyreset())
  • a new keycache entry is allocated for a different node (ath_hal_keyset())
  • a frame from the freed node is received prior to or sometime in the meantime of the actions described above

A stale ds_rxstat.rs_keyix causes obtaining wrong struct ieee80211_node * from sc_keyixmap, which is then passed to net80211 stack.

The patch is a workaround for this problem rather than an ultimate solution. IMHO the place to perform rs_keyix invalidation is the hal.

http: //dev.lintrack.org/browser/trunk/packages/madwifi/patches/012-keycache_check_rxnode.diff.bz2?format=raw

Signed-off-by: Michal Wrobel <xmxwx@asn.pl>

Attachments

012-keycache_check_rxnode.diff (2.9 kB) - added by xmxwx@asn.pl on 10/19/06 01:49:42.
012-keycache_check_rxnode.2.diff (2.0 kB) - added by mtaylor on 05/24/07 23:57:29.
Updated to apply to trunk.

Change History

10/19/06 00:11:24 changed by kelmo

Hi Michal,

I am not sure about the other developers, but submittimg patches in this manner is a royal PITA. Not only do i need to cut + paste urls to other trac ticket systems into my browser, but i must also download and unpack the diff.bz2 to even take a look at the changes mentioned on the third-party bug tracker.

All the URL's given also have a bogus space after http://.

Please take the time to describe and attach the patches, in raw unified diff format, to this bug tracker so that we may better retain history of changes to the driver.

This comment applies to #962 #963 #964 #965 #966 #967

Thanks, Kel.

10/19/06 01:20:27 changed by xmxwx@asn.pl

Hi Kel,

I'd like to apologize for any inconvenience I've caused by submitting patches in that manner. However, there are reasons why I have done it so.

All the URL's given also have a bogus space after (...)

This is intentional. This is not my idea, though. Everytime I wanted to paste sane URLs I got the 403 message with a suggestion how to obfuscate URLs to omit the spam blocker. I just followed the "Example: http :/".

Not only do i need to cut + paste urls to other trac ticket systems into my browser, but i must also download and unpack the diff.bz2 to even take a look at the changes mentioned on the third-party bug tracker.

The need of cutting and pasting the URLs is the drawback of the spam blocker, as I explained above; bzip2 compression follows the natural scheme of the distribution these patches are part of; third-party bug tracker (i.e. outside URL, not an attachment) is because the patches were initially not targetted at a quick inclusion to the mainstream madwifi development tree. However, because the natural thing in the Open Source community is to share one's work with the others, I decided to publish the patches here, so that everyone can take an advantage of them. I wanted to make it fast, without excessive effort, but unfortunately it eventually appeared not as easy as it seemed. So, once again - sorry for inflicting any P (let alone the place it occured).

I'll take the time and attach the patches instead of linking. I hope that descriptions at the beginning of the patches are not a problem? Are they?

Regards, Michal.

10/19/06 01:26:43 changed by kelmo

Thats fine, so long as we don't have to go fishing for the actual code.

I'm excited you are contributing back to MadWifi. These patches could be merged and discussed much easier if there were accessable in methods conventional to our setup.

10/19/06 01:49:42 changed by xmxwx@asn.pl

  • attachment 012-keycache_check_rxnode.diff added.

10/19/06 01:59:07 changed by xmxwx@asn.pl

Besides the tickets you've mentioned, patches have been attached to #113, #511, #858.

10/19/06 13:05:09 changed by mrenzmann

  • version set to v0.9.2.
  • milestone set to version 0.9.x - progressive release candidate phase.

Thanks for the contribution.

I'm aware that the spam blocker might be wrong in cases like this. However, the site is currently hit by ~400 spam comments per day, which by far outweights the disadvantages of not being allowed to give fully working links in ticket comments. Apart from that we generally prefer to have referenced files (especially patches) attached to the corresponding ticket, since that makes the reviewing process independant of other servers and allows to make use of some of Trac's functionality.

05/22/07 06:25:22 changed by mrenzmann

Wondering: is there a reason for the /*&& !IS_RTS(ah)*/ in chunk 2?

05/24/07 23:57:29 changed by mtaylor

  • attachment 012-keycache_check_rxnode.2.diff added.

Updated to apply to trunk.

05/25/07 00:13:06 changed by mtaylor

(15:10:52) mentor: OK, so I would generalise the two IS_FOO macros so they can be used anywhere (15:11:25) mentor: I would write a function to go from the wireless header to RA, TA, DA, SA, BSS (15:11:53) mentor: And I would work out why the wrong data is in the keyixmap in the first place...

05/25/07 01:35:07 changed by mtaylor

After discussion with Mentor, we decided to add locking to the ath_key_* family of functions to synchronize access to the key cache. Then, we will update the ath_key_delete function to invalidate any inbound descriptors pending in the rx queue (and having that key index) so that the rx_tasklet will drop them.

While the ath_key_update_begin/ath_key_update_end methods already stop the receive tasklet, it is possible that another processor incidentally causes it to be restarted.

While ath_key_delete holds the lock on the key cache, it will also acquire a lock to prevent access to the receive queue data buffers for long enough to make it's mark pass through the queue.