Re: paged RX skbs and BlockAck Request packets

[Johannes Berg <johannes@sipsolutions.net>]

On Fri, 2010-05-28 at 12:46 -0700, Daniel Halperin wrote:
> I'm using the latest iwlwifi-2.6.git from
> http://git.kernel.org/?p=linux/kernel/git/iwlwifi/iwlwifi-2.6.git
> 
> When using 802.11n aggregation and the other endpoint sends a BlockAck
> Request, many times the transfer will completely stall.
> 
> It looks like the relevant code is in
> net/mac80211/rx.c:ieee80211_rx_h_ctrl . I found that the sequence
> numbers used are invalid. If instead I linearize the SKB, then the
> sequence numbers become valid, so I believe it's a problem with the
> use of paged RX skbs. Mailing both lists since I'm not sure where the
> fix should go.
> 
> The paged RX SKBs are set up in
> drivers/net/wireless/iwlwifi:iwl-agn-lib.c:iwl_pass_packet_to_mac80211. I made a temporary fix at net/mac80211/rx.c:__ieee80211_rx_handle_packet
> 
> by changing 
> 
>         if (ieee80211_is_mgmt(fc))
>                 err = skb_linearize(skb);
> 
> to
> 
>         if (ieee80211_is_mgmt(fc) || ieee80211_is_ctl(fc))
>                 err = skb_linearize(skb);
> 
> Can anyone more knowledgeable than I please tell me the right fix?

Wow, good catch. FWIW, that seems like a perfectly appropriate fix,
since control frames are typically very short I don't see any problem in
linearizing them, in fact I'd think the skb should already have enough
space at this point. If you wanted to avoid that, you need a patch like
the one below.

One thing I ask myself though is do we ever check that the frame is long
enough? In the patch below I will by checking the skb_copy_bits() return
value, but without that we don't, as far as I can tell?

johannes

--- wireless-testing.orig/net/mac80211/rx.c	2010-05-28 22:25:07.000000000 +0200
+++ wireless-testing/net/mac80211/rx.c	2010-05-28 22:33:30.000000000 +0200
@@ -1819,17 +1819,26 @@ ieee80211_rx_h_ctrl(struct ieee80211_rx_
 		return RX_CONTINUE;
 
 	if (ieee80211_is_back_req(bar->frame_control)) {
+		struct {
+			__le16 control, start_seq_num;
+		} __packed bar_data;
+
+		if (skb_copy_bits(skb, offsetof(struct ieee80211_bar, control),
+				  &bar_data, sizeof(bar_data)))
+			return RX_DROP_MONITOR;
+
 		if (!rx->sta)
 			return RX_DROP_MONITOR;
+
 		spin_lock(&rx->sta->lock);
-		tid = le16_to_cpu(bar->control) >> 12;
+		tid = le16_to_cpu(bar_data.control) >> 12;
 		if (!rx->sta->ampdu_mlme.tid_active_rx[tid]) {
 			spin_unlock(&rx->sta->lock);
 			return RX_DROP_MONITOR;
 		}
 		tid_agg_rx = rx->sta->ampdu_mlme.tid_rx[tid];
 
-		start_seq_num = le16_to_cpu(bar->start_seq_num) >> 4;
+		start_seq_num = le16_to_cpu(bar_data.start_seq_num) >> 4;
 
 		/* reset session timer */
 		if (tid_agg_rx->timeout)