On Fri, 2010-10-08 at 18:42 +0200, Christian Lamparter wrote:
> Sure, a little bit. The code itself is fine but as you said
> the rx_handler code wasn't written for concurrent/delayed
> release timer mechanism.
But it should be fine now, no? What data does it still access that's not
safe?
> for example:
>
> Because we can't set IEEE80211_RX_RA_MATCH (since
> it interferes with scanning (as explained in
> "mac80211: fix release_reorder_timeout in scan").
That I don't understand.
> We will experience strange results with "ieee80211_rx_h_decrypt":
>
> line: 878
> > /*
> > * No point in finding a key and decrypting if the frame is neither
> > * addressed to us nor a multicast frame.
> > */
> > if (!(status->rx_flags & IEEE80211_RX_RA_MATCH))
> no software decryption there - not nice but the HW probably does
> the decryption for us. - That being said, the stack should be able
> to do the software decryption "just in case".
But note that the rx_flags are in the *status* now, which is part of the
SKB, and no longer on the stack.
> Things are a little bit better with ieee80211_rx_h_sta_process.
> It updates some statistics and takes care of sta->last_rx
> (which is currently not that important giving HT BA is only supported
> for AP/STA operation).
>
> In ieee80211_rx_h_data, there could be another potential problem:
> > if (ieee80211_is_data(hdr->frame_control) &&
> > !is_multicast_ether_addr(hdr->addr1) &&
> > local->hw.conf.dynamic_ps_timeout > 0 && local->ps_sdata) {
> > mod_timer(&local->dynamic_ps_timer, jiffies +
> > msecs_to_jiffies(local->hw.conf.dynamic_ps_timeout));
> > }
> I reckon there could be a "hidden" problem. "jiffies" is now
> approx 100ms after the packet was received from the interface.
> (Sure, a similar issue was also present in the original
> reorder release implementation.)
This one's more interesting. I guess we need to bypass these things
somehow, maybe setting a flag if this was a "recovered" frame?
> In order the fix this/my mess we would need to:
> 1. move the software decryption before the reordering
> (802.11n-spec (page 11, Figure 6-1) allows this)
>
> (Or:
> 1. introduce an additional rx_flag for the reorder release case?)
>
> (2. maybe cache the original skb jiffie at some place?)
>
> (3. make a few counters atomic_t, so concurrent tasklets
> can update the stats. Or disable the BHs while processing,
> any rx frames (which is probably what we're going to do, right?))
BHs are disabled while processing RX -- and timer is a BH itself so
they're also disabled, right?
johannes
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
