[PATCH] mac80211: Fix PN corruption in case of multiple virtual interface

[PATCH] mac80211: Fix PN corruption in case of multiple virtual interface

[Amit Shakya]

In case of multiple virtual interface, if AES is configured on multiple
interface then there are chances of stored PN corruption, causing traffic
to stop.
In case, ieee80211_rx_handlers processing is going on for skbs received on
one vif and at the same time, rx aggregation reorder timer expires on
another vif then sta_rx_agg_reorder_timer_expired is invoked and it will
push skbs into the single queue (local->rx_skb_queue).
ieee80211_rx_handlers in the while loop assumes that the skbs are for the
same TID and same sta. This assumption doesn't hold good in this scenario
and the PN gets corrupted by PN received in other vif's skb, causing
traffic to stop due to PN mismatch.
This can be avoided by comparing source mac addres in received skb's with
the sta's mac address for which processing is going on, when de-queueing.

Signed-off-by: Amit Shakya <amit.shakya@stericsson.com>
---
 net/mac80211/rx.c |   18 ++++++++++++++++--
 1 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 580704e..e6f1799 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -2775,7 +2775,8 @@ static void ieee80211_rx_handlers_result(struct ieee80211_rx_data *rx,
 static void ieee80211_rx_handlers(struct ieee80211_rx_data *rx)
 {
 	ieee80211_rx_result res = RX_DROP_MONITOR;
-	struct sk_buff *skb;
+	struct sk_buff *skb, *tmp;
+	struct ieee80211_hdr *hdr;
 
 #define CALL_RXH(rxh)			\
 	do {				\
@@ -2790,7 +2791,20 @@ static void ieee80211_rx_handlers(struct ieee80211_rx_data *rx)
 
 	rx->local->running_rx_handler = true;
 
-	while ((skb = __skb_dequeue(&rx->local->rx_skb_queue))) {
+	skb_queue_walk_safe(&rx->local->rx_skb_queue, skb, tmp) {
+		if (!skb)
+			break;
+		hdr = (struct ieee80211_hdr *) skb->data;
+		/*
+		* Additional check to ensure that the packets corresponding
+		* to same sta entry as in rx->sta are de-queued. The queue
+		* can have different interface packets in case of multiple vifs
+		*/
+		if ((rx->sta && hdr) && (ieee80211_is_data(hdr->frame_control))
+			&& (memcmp(rx->sta->sta.addr, hdr->addr2, ETH_ALEN)))
+					continue;
+		__skb_unlink(skb, &rx->local->rx_skb_queue);
+
 		spin_unlock(&rx->local->rx_skb_queue.lock);
 
 		/*
-- 
1.7.4.1

Re: [PATCH] mac80211: Fix PN corruption in case of multiple virtual interface

[Johannes Berg]

On Mon, 2013-02-04 at 16:48 +0530, Amit Shakya wrote:
> In case of multiple virtual interface, if AES is configured on multiple
> interface then there are chances of stored PN corruption, causing traffic
> to stop.

Interesting, nice find.

> In case, ieee80211_rx_handlers processing is going on for skbs received on
> one vif and at the same time, rx aggregation reorder timer expires on
> another vif then sta_rx_agg_reorder_timer_expired is invoked and it will
> push skbs into the single queue (local->rx_skb_queue).
> ieee80211_rx_handlers in the while loop assumes that the skbs are for the
> same TID and same sta. 

This is because of struct ieee80211_rx_data, presumably? IOW, the loop
doesn't really assume it, it's the fact that the loop is called with a
given struct ieee80211_rx_data, right?

> This assumption doesn't hold good in this scenario
> and the PN gets corrupted by PN received in other vif's skb, causing
> traffic to stop due to PN mismatch.
> This can be avoided by comparing source mac addres in received skb's with
> the sta's mac address for which processing is going on, when de-queueing.



> @@ -2790,7 +2791,20 @@ static void ieee80211_rx_handlers(struct ieee80211_rx_data *rx)
>  
>  	rx->local->running_rx_handler = true;
>  
> -	while ((skb = __skb_dequeue(&rx->local->rx_skb_queue))) {
> +	skb_queue_walk_safe(&rx->local->rx_skb_queue, skb, tmp) {
> +		if (!skb)
> +			break;
> +		hdr = (struct ieee80211_hdr *) skb->data;
> +		/*
> +		* Additional check to ensure that the packets corresponding
> +		* to same sta entry as in rx->sta are de-queued. The queue
> +		* can have different interface packets in case of multiple vifs
> +		*/
> +		if ((rx->sta && hdr) && (ieee80211_is_data(hdr->frame_control))
> +			&& (memcmp(rx->sta->sta.addr, hdr->addr2, ETH_ALEN)))
> +					continue;
> +		__skb_unlink(skb, &rx->local->rx_skb_queue);

Apart from the curious coding style (you don't need any of those extra
parentheses, && should be on the previous line, the if continuation
indented to the opening parenthesis, continue only indented one tab), I
wonder if this could lead to leaking frames here, if the station
disconnects or something while there are frames for it on the queue?
IOW, the "just skip that frame" piece seems a bit questionable.

Christian, is there any reason to not just have the queue be on the
stack, and use a separate spinlock in the local struct to lock out the
unwanted concurrency? It seems to me that should work just as well,
since there are never frames on the rx_skb_queue for very long, right?

johannes

Re: [PATCH] mac80211: Fix PN corruption in case of multiple virtual interface

[Christian Lamparter]

On Monday, February 04, 2013 04:28:28 PM Johannes Berg wrote:
> On Mon, 2013-02-04 at 16:48 +0530, Amit Shakya wrote:
> > @@ -2790,7 +2791,20 @@ static void ieee80211_rx_handlers(struct ieee80211_rx_data *rx)
> >  
> >  	rx->local->running_rx_handler = true;
> >  
> > -	while ((skb = __skb_dequeue(&rx->local->rx_skb_queue))) {
> > +	skb_queue_walk_safe(&rx->local->rx_skb_queue, skb, tmp) {
> > +		if (!skb)
> > +			break;
> > +		hdr = (struct ieee80211_hdr *) skb->data;
> > +		/*
> > +		* Additional check to ensure that the packets corresponding
> > +		* to same sta entry as in rx->sta are de-queued. The queue
> > +		* can have different interface packets in case of multiple vifs
> > +		*/
> > +		if ((rx->sta && hdr) && (ieee80211_is_data(hdr->frame_control))
> > +			&& (memcmp(rx->sta->sta.addr, hdr->addr2, ETH_ALEN)))
> > +					continue;
> > +		__skb_unlink(skb, &rx->local->rx_skb_queue);
> 
> Christian, is there any reason to not just have the queue be on the
> stack, and use a separate spinlock in the local struct to lock out the
> unwanted concurrency?
Let's see.

The original "AMPDU rx reorder timeout timer" had the rx_skb_queue (frames)
on the stack. But that didn't work because the rx-path isn't thread-safe. This
issue was addressed by "mac80211: serialize rx path workers" (24a8fda). 
Interestingly, the RFC [1] of this patch mentioned the reason why I/we didn't
go for a rx-path lock:
"	1. Locking is easy to implement but hard to maintain.
	   Furthermore, Johannes worked very hard to get rid
	   of as many as possible."

> It seems to me that should work just as well, since there are never frames
> on the rx_skb_queue for very long, right?
Yes it should. At least we didn't find anything wrong with it back then.

Regards,
	Christian

[1] <http://article.gmane.org/gmane.linux.kernel.wireless.general/62240>

Re: [PATCH] mac80211: Fix PN corruption in case of multiple virtual interface

[Johannes Berg]

On Mon, 2013-02-04 at 18:14 +0100, Christian Lamparter wrote:
> On Monday, February 04, 2013 04:28:28 PM Johannes Berg wrote:
> > On Mon, 2013-02-04 at 16:48 +0530, Amit Shakya wrote:
> > > @@ -2790,7 +2791,20 @@ static void ieee80211_rx_handlers(struct ieee80211_rx_data *rx)
> > >  
> > >     rx->local->running_rx_handler = true;
> > >  
> > > -   while ((skb = __skb_dequeue(&rx->local->rx_skb_queue))) {
> > > +   skb_queue_walk_safe(&rx->local->rx_skb_queue, skb, tmp) {
> > > +           if (!skb)
> > > +                   break;
> > > +           hdr = (struct ieee80211_hdr *) skb->data;
> > > +           /*
> > > +           * Additional check to ensure that the packets corresponding
> > > +           * to same sta entry as in rx->sta are de-queued. The queue
> > > +           * can have different interface packets in case of multiple vifs
> > > +           */
> > > +           if ((rx->sta && hdr) && (ieee80211_is_data(hdr->frame_control))
> > > +                   && (memcmp(rx->sta->sta.addr, hdr->addr2, ETH_ALEN)))
> > > +                                   continue;
> > > +           __skb_unlink(skb, &rx->local->rx_skb_queue);
> > 
> > Christian, is there any reason to not just have the queue be on the
> > stack, and use a separate spinlock in the local struct to lock out the
> > unwanted concurrency?

> Let's see.
> 
> The original "AMPDU rx reorder timeout timer" had the rx_skb_queue (frames)
> on the stack. But that didn't work because the rx-path isn't thread-safe. This
> issue was addressed by "mac80211: serialize rx path workers" (24a8fda). 

It seems this actually caused the problem, because this part:

    Only one active rx handler worker [ieee80211_rx_handlers]
    is needed. All other threads which have lost the race of
    "runnning_rx_handler" can now simply "return", knowing that
    the thread who had the "edge" will also take care of their
    workload.

forgot to account for the fact that the on-stack versions of "struct
ieee80211_rx_data" can be different. Right?

> Interestingly, the RFC [1] of this patch mentioned the reason why I/we didn't
> go for a rx-path lock:
> "       1. Locking is easy to implement but hard to maintain.
>            Furthermore, Johannes worked very hard to get rid
>            of as many as possible."
> 
> > It seems to me that should work just as well, since there are never frames
> > on the rx_skb_queue for very long, right?
> Yes it should. At least we didn't find anything wrong with it back then.

But ... that doesn't necessarily mean an RX path lock, does it? 

I mean, in order to fix the above, we *do* have to make the RX
tasklet/timer wait for each other. So it's not really a big difference
between what we do now and having one of them block, is it? I guess that
they can still do all the local work, and then call the RX handlers with
the lock held? Hmm. That does kinda mean an RX path lock :-)

I guess it's the only way I see, since we can't really disable RX from
drivers when the timer starts running.

johannes

Re: [PATCH] mac80211: Fix PN corruption in case of multiple virtual interface

[Christian Lamparter]

On Monday, February 04, 2013 06:30:18 PM Johannes Berg wrote:
> On Mon, 2013-02-04 at 18:14 +0100, Christian Lamparter wrote:
> > On Monday, February 04, 2013 04:28:28 PM Johannes Berg wrote:
> > > On Mon, 2013-02-04 at 16:48 +0530, Amit Shakya wrote:
> > > > @@ -2790,7 +2791,20 @@ static void ieee80211_rx_handlers(struct ieee80211_rx_data *rx)
> > > >  
> > > >     rx->local->running_rx_handler = true;
> > > >  
> > > > -   while ((skb = __skb_dequeue(&rx->local->rx_skb_queue))) {
> > > > +   skb_queue_walk_safe(&rx->local->rx_skb_queue, skb, tmp) {
> > > > +           if (!skb)
> > > > +                   break;
> > > > +           hdr = (struct ieee80211_hdr *) skb->data;
> > > > +           /*
> > > > +           * Additional check to ensure that the packets corresponding
> > > > +           * to same sta entry as in rx->sta are de-queued. The queue
> > > > +           * can have different interface packets in case of multiple vifs
> > > > +           */
> > > > +           if ((rx->sta && hdr) && (ieee80211_is_data(hdr->frame_control))
> > > > +                   && (memcmp(rx->sta->sta.addr, hdr->addr2, ETH_ALEN)))
> > > > +                                   continue;
> > > > +           __skb_unlink(skb, &rx->local->rx_skb_queue);
> > > 
> > > Christian, is there any reason to not just have the queue be on the
> > > stack, and use a separate spinlock in the local struct to lock out the
> > > unwanted concurrency?
> 
> > Let's see.
> > 
> > The original "AMPDU rx reorder timeout timer" had the rx_skb_queue (frames)
> > on the stack. But that didn't work because the rx-path isn't thread-safe. This
> > issue was addressed by "mac80211: serialize rx path workers" (24a8fda). 
> 
> It seems this actually caused the problem, because this part:
> 
>     Only one active rx handler worker [ieee80211_rx_handlers]
>     is needed. All other threads which have lost the race of
>     "runnning_rx_handler" can now simply "return", knowing that
>     the thread who had the "edge" will also take care of their
>     workload.
> 
> forgot to account for the fact that the on-stack versions of "struct
> ieee80211_rx_data" can be different. Right?
Yes, sdata and sta can be different.

> > Interestingly, the RFC [1] of this patch mentioned the reason why I/we didn't
> > go for a rx-path lock:
> > "       1. Locking is easy to implement but hard to maintain.
> >            Furthermore, Johannes worked very hard to get rid
> >            of as many as possible."
> > 
> > > It seems to me that should work just as well, since there are never frames
> > > on the rx_skb_queue for very long, right?
> > Yes it should. At least we didn't find anything wrong with it back then.
> 
> But ... that doesn't necessarily mean an RX path lock, does it? 
> 
> I mean, in order to fix the above, we *do* have to make the RX
> tasklet/timer wait for each other. So it's not really a big difference
> between what we do now and having one of them block, is it? I guess that
> they can still do all the local work, and then call the RX handlers with
> the lock held? Hmm. That does kinda mean an RX path lock :-)

See the attached patch. It compiles, but I can't test it right now.

Regards,
	Christian

---
mac80211: protect rx-path with spinlock

(Text is from "Fix PN corruption in case of multiple virtual interface"
with a little modification => "same sdata and sta")

In case, ieee80211_rx_handlers processing is going on for skbs received on
one vif and at the same time, rx aggregation reorder timer expires on
another vif then sta_rx_agg_reorder_timer_expired is invoked and it will
push skbs into the single queue (local->rx_skb_queue).
ieee80211_rx_handlers in the while loop assumes that the skbs are for the
same sdata and sta. This assumption doesn't hold good in this scenario
and the PN gets corrupted by PN received in other vif's skb, causing
traffic to stop due to PN mismatch.

---
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 5fba867..11a520b 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -986,14 +986,7 @@ struct ieee80211_local {
 	struct sk_buff_head skb_queue;
 	struct sk_buff_head skb_queue_unreliable;
 
-	/*
-	 * Internal FIFO queue which is shared between multiple rx path
-	 * stages. Its main task is to provide a serialization mechanism,
-	 * so all rx handlers can enjoy having exclusive access to their
-	 * private data structures.
-	 */
-	struct sk_buff_head rx_skb_queue;
-	bool running_rx_handler;	/* protected by rx_skb_queue.lock */
+	spinlock_t rx_handler;
 
 	/* Station data */
 	/*
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 2bdd454..6517dd5 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -34,8 +34,6 @@
 #include "cfg.h"
 #include "debugfs.h"
 
-static struct lock_class_key ieee80211_rx_skb_queue_class;
-
 void ieee80211_configure_filter(struct ieee80211_local *local)
 {
 	u64 mc;
@@ -613,21 +611,12 @@ struct ieee80211_hw *ieee80211_alloc_hw(size_t priv_data_len,
 
 	mutex_init(&local->key_mtx);
 	spin_lock_init(&local->filter_lock);
+	spin_lock_init(&local->rx_handler);
 	spin_lock_init(&local->queue_stop_reason_lock);
 
 	INIT_LIST_HEAD(&local->chanctx_list);
 	mutex_init(&local->chanctx_mtx);
 
-	/*
-	 * The rx_skb_queue is only accessed from tasklets,
-	 * but other SKB queues are used from within IRQ
-	 * context. Therefore, this one needs a different
-	 * locking class so our direct, non-irq-safe use of
-	 * the queue's lock doesn't throw lockdep warnings.
-	 */
-	skb_queue_head_init_class(&local->rx_skb_queue,
-				  &ieee80211_rx_skb_queue_class);
-
 	INIT_DELAYED_WORK(&local->scan_work, ieee80211_scan_work);
 
 	INIT_WORK(&local->restart_work, ieee80211_restart_work);
@@ -1089,7 +1078,6 @@ void ieee80211_unregister_hw(struct ieee80211_hw *hw)
 		wiphy_warn(local->hw.wiphy, "skb_queue not empty\n");
 	skb_queue_purge(&local->skb_queue);
 	skb_queue_purge(&local->skb_queue_unreliable);
-	skb_queue_purge(&local->rx_skb_queue);
 
 	destroy_workqueue(local->workqueue);
 	wiphy_unregister(local->hw.wiphy);
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index a190895..c52b15d 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -668,9 +668,9 @@ static inline u16 seq_sub(u16 sq1, u16 sq2)
 
 static void ieee80211_release_reorder_frame(struct ieee80211_sub_if_data *sdata,
 					    struct tid_ampdu_rx *tid_agg_rx,
-					    int index)
+					    int index,
+					    struct sk_buff_head *frames)
 {
-	struct ieee80211_local *local = sdata->local;
 	struct sk_buff *skb = tid_agg_rx->reorder_buf[index];
 	struct ieee80211_rx_status *status;
 
@@ -684,7 +684,7 @@ static void ieee80211_release_reorder_frame(struct ieee80211_sub_if_data *sdata,
 	tid_agg_rx->reorder_buf[index] = NULL;
 	status = IEEE80211_SKB_RXCB(skb);
 	status->rx_flags |= IEEE80211_RX_DEFERRED_RELEASE;
-	skb_queue_tail(&local->rx_skb_queue, skb);
+	__skb_queue_tail(frames, skb);
 
 no_frame:
 	tid_agg_rx->head_seq_num = seq_inc(tid_agg_rx->head_seq_num);
@@ -692,7 +692,8 @@ no_frame:
 
 static void ieee80211_release_reorder_frames(struct ieee80211_sub_if_data *sdata,
 					     struct tid_ampdu_rx *tid_agg_rx,
-					     u16 head_seq_num)
+					     u16 head_seq_num,
+					     struct sk_buff_head *frames)
 {
 	int index;
 
@@ -701,7 +702,8 @@ static void ieee80211_release_reorder_frames(struct ieee80211_sub_if_data *sdata
 	while (seq_less(tid_agg_rx->head_seq_num, head_seq_num)) {
 		index = seq_sub(tid_agg_rx->head_seq_num, tid_agg_rx->ssn) %
 							tid_agg_rx->buf_size;
-		ieee80211_release_reorder_frame(sdata, tid_agg_rx, index);
+		ieee80211_release_reorder_frame(sdata, tid_agg_rx, index,
+						frames);
 	}
 }
 
@@ -717,7 +719,8 @@ static void ieee80211_release_reorder_frames(struct ieee80211_sub_if_data *sdata
 #define HT_RX_REORDER_BUF_TIMEOUT (HZ / 10)
 
 static void ieee80211_sta_reorder_release(struct ieee80211_sub_if_data *sdata,
-					  struct tid_ampdu_rx *tid_agg_rx)
+					  struct tid_ampdu_rx *tid_agg_rx,
+					  struct sk_buff_head *frames)
 {
 	int index, j;
 
@@ -746,7 +749,8 @@ static void ieee80211_sta_reorder_release(struct ieee80211_sub_if_data *sdata,
 
 			ht_dbg_ratelimited(sdata,
 					   "release an RX reorder frame due to timeout on earlier frames\n");
-			ieee80211_release_reorder_frame(sdata, tid_agg_rx, j);
+			ieee80211_release_reorder_frame(sdata, tid_agg_rx, j,
+							frames);
 
 			/*
 			 * Increment the head seq# also for the skipped slots.
@@ -756,7 +760,8 @@ static void ieee80211_sta_reorder_release(struct ieee80211_sub_if_data *sdata,
 			skipped = 0;
 		}
 	} else while (tid_agg_rx->reorder_buf[index]) {
-		ieee80211_release_reorder_frame(sdata, tid_agg_rx, index);
+		ieee80211_release_reorder_frame(sdata, tid_agg_rx, index,
+						frames);
 		index =	seq_sub(tid_agg_rx->head_seq_num, tid_agg_rx->ssn) %
 							tid_agg_rx->buf_size;
 	}
@@ -788,7 +793,8 @@ static void ieee80211_sta_reorder_release(struct ieee80211_sub_if_data *sdata,
  */
 static bool ieee80211_sta_manage_reorder_buf(struct ieee80211_sub_if_data *sdata,
 					     struct tid_ampdu_rx *tid_agg_rx,
-					     struct sk_buff *skb)
+					     struct sk_buff *skb,
+					     struct sk_buff_head *frames)
 {
 	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb->data;
 	u16 sc = le16_to_cpu(hdr->seq_ctrl);
@@ -816,7 +822,7 @@ static bool ieee80211_sta_manage_reorder_buf(struct ieee80211_sub_if_data *sdata
 		head_seq_num = seq_inc(seq_sub(mpdu_seq_num, buf_size));
 		/* release stored frames up to new head to stack */
 		ieee80211_release_reorder_frames(sdata, tid_agg_rx,
-						 head_seq_num);
+						 head_seq_num, frames);
 	}
 
 	/* Now the new frame is always in the range of the reordering buffer */
@@ -846,7 +852,7 @@ static bool ieee80211_sta_manage_reorder_buf(struct ieee80211_sub_if_data *sdata
 	tid_agg_rx->reorder_buf[index] = skb;
 	tid_agg_rx->reorder_time[index] = jiffies;
 	tid_agg_rx->stored_mpdu_num++;
-	ieee80211_sta_reorder_release(sdata, tid_agg_rx);
+	ieee80211_sta_reorder_release(sdata, tid_agg_rx, frames);
 
  out:
 	spin_unlock(&tid_agg_rx->reorder_lock);
@@ -857,7 +863,8 @@ static bool ieee80211_sta_manage_reorder_buf(struct ieee80211_sub_if_data *sdata
  * Reorder MPDUs from A-MPDUs, keeping them on a buffer. Returns
  * true if the MPDU was buffered, false if it should be processed.
  */
-static void ieee80211_rx_reorder_ampdu(struct ieee80211_rx_data *rx)
+static void ieee80211_rx_reorder_ampdu(struct ieee80211_rx_data *rx,
+				       struct sk_buff_head *frames)
 {
 	struct sk_buff *skb = rx->skb;
 	struct ieee80211_local *local = rx->local;
@@ -922,11 +929,12 @@ static void ieee80211_rx_reorder_ampdu(struct ieee80211_rx_data *rx)
 	 * sure that we cannot get to it any more before doing
 	 * anything with it.
 	 */
-	if (ieee80211_sta_manage_reorder_buf(rx->sdata, tid_agg_rx, skb))
+	if (ieee80211_sta_manage_reorder_buf(rx->sdata, tid_agg_rx, skb,
+					     frames))
 		return;
 
  dont_reorder:
-	skb_queue_tail(&local->rx_skb_queue, skb);
+	__skb_queue_tail(frames, skb);
 }
 
 static ieee80211_rx_result debug_noinline
@@ -2177,7 +2185,7 @@ ieee80211_rx_h_data(struct ieee80211_rx_data *rx)
 }
 
 static ieee80211_rx_result debug_noinline
-ieee80211_rx_h_ctrl(struct ieee80211_rx_data *rx)
+ieee80211_rx_h_ctrl(struct ieee80211_rx_data *rx, struct sk_buff_head *frames)
 {
 	struct sk_buff *skb = rx->skb;
 	struct ieee80211_bar *bar = (struct ieee80211_bar *)skb->data;
@@ -2216,7 +2224,7 @@ ieee80211_rx_h_ctrl(struct ieee80211_rx_data *rx)
 		spin_lock(&tid_agg_rx->reorder_lock);
 		/* release stored frames up to start of BAR */
 		ieee80211_release_reorder_frames(rx->sdata, tid_agg_rx,
-						 start_seq_num);
+						 start_seq_num, frames);
 		spin_unlock(&tid_agg_rx->reorder_lock);
 
 		kfree_skb(skb);
@@ -2801,7 +2809,8 @@ static void ieee80211_rx_handlers_result(struct ieee80211_rx_data *rx,
 	}
 }
 
-static void ieee80211_rx_handlers(struct ieee80211_rx_data *rx)
+static void ieee80211_rx_handlers(struct ieee80211_rx_data *rx,
+				  struct sk_buff_head *frames)
 {
 	ieee80211_rx_result res = RX_DROP_MONITOR;
 	struct sk_buff *skb;
@@ -2813,15 +2822,9 @@ static void ieee80211_rx_handlers(struct ieee80211_rx_data *rx)
 			goto rxh_next;  \
 	} while (0);
 
-	spin_lock(&rx->local->rx_skb_queue.lock);
-	if (rx->local->running_rx_handler)
-		goto unlock;
-
-	rx->local->running_rx_handler = true;
-
-	while ((skb = __skb_dequeue(&rx->local->rx_skb_queue))) {
-		spin_unlock(&rx->local->rx_skb_queue.lock);
+	spin_lock_bh(&rx->local->rx_handler);
 
+	while ((skb = __skb_dequeue(frames))) {
 		/*
 		 * all the other fields are valid across frames
 		 * that belong to an aMPDU since they are on the
@@ -2842,7 +2845,12 @@ static void ieee80211_rx_handlers(struct ieee80211_rx_data *rx)
 #endif
 		CALL_RXH(ieee80211_rx_h_amsdu)
 		CALL_RXH(ieee80211_rx_h_data)
-		CALL_RXH(ieee80211_rx_h_ctrl);
+
+		/* special treatment -- needs the queue */
+		res = ieee80211_rx_h_ctrl(rx, frames);
+		if (res != RX_CONTINUE)
+			goto rxh_next;
+
 		CALL_RXH(ieee80211_rx_h_mgmt_check)
 		CALL_RXH(ieee80211_rx_h_action)
 		CALL_RXH(ieee80211_rx_h_userspace_mgmt)
@@ -2851,20 +2859,20 @@ static void ieee80211_rx_handlers(struct ieee80211_rx_data *rx)
 
  rxh_next:
 		ieee80211_rx_handlers_result(rx, res);
-		spin_lock(&rx->local->rx_skb_queue.lock);
+
 #undef CALL_RXH
 	}
 
-	rx->local->running_rx_handler = false;
-
- unlock:
-	spin_unlock(&rx->local->rx_skb_queue.lock);
+	spin_unlock_bh(&rx->local->rx_handler);
 }
 
 static void ieee80211_invoke_rx_handlers(struct ieee80211_rx_data *rx)
 {
+	struct sk_buff_head reorder_release;
 	ieee80211_rx_result res = RX_DROP_MONITOR;
 
+	__skb_queue_head_init(&reorder_release);
+
 #define CALL_RXH(rxh)			\
 	do {				\
 		res = rxh(rx);		\
@@ -2874,9 +2882,9 @@ static void ieee80211_invoke_rx_handlers(struct ieee80211_rx_data *rx)
 
 	CALL_RXH(ieee80211_rx_h_check)
 
-	ieee80211_rx_reorder_ampdu(rx);
+	ieee80211_rx_reorder_ampdu(rx, &reorder_release);
 
-	ieee80211_rx_handlers(rx);
+	ieee80211_rx_handlers(rx, &reorder_release);
 	return;
 
  rxh_next:
@@ -2891,6 +2899,7 @@ static void ieee80211_invoke_rx_handlers(struct ieee80211_rx_data *rx)
  */
 void ieee80211_release_reorder_timeout(struct sta_info *sta, int tid)
 {
+	struct sk_buff_head frames;
 	struct ieee80211_rx_data rx = {
 		.sta = sta,
 		.sdata = sta->sdata,
@@ -2906,11 +2915,13 @@ void ieee80211_release_reorder_timeout(struct sta_info *sta, int tid)
 	if (!tid_agg_rx)
 		return;
 
+	__skb_queue_head_init(&frames);
+
 	spin_lock(&tid_agg_rx->reorder_lock);
-	ieee80211_sta_reorder_release(sta->sdata, tid_agg_rx);
+	ieee80211_sta_reorder_release(sta->sdata, tid_agg_rx, &frames);
 	spin_unlock(&tid_agg_rx->reorder_lock);
 
-	ieee80211_rx_handlers(&rx);
+	ieee80211_rx_handlers(&rx, &frames);
 }
 
 /* main receive path */

Re: [PATCH] mac80211: Fix PN corruption in case of multiple virtual interface

[Johannes Berg]

On Mon, 2013-02-04 at 18:44 +0100, Christian Lamparter wrote:

> See the attached patch. It compiles, but I can't test it right now.

> mac80211: protect rx-path with spinlock
> 
[...]

Looks good to me. Amit, can you test it please?

johannes

RE: [PATCH] mac80211: Fix PN corruption in case of multiple virtual interface

[Amit SHAKYA]

TXkgY29tbWVudHMgaW5saW5lW0FTXToNCg0KLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCkZy
b206IEpvaGFubmVzIEJlcmcgW21haWx0bzpqb2hhbm5lc0BzaXBzb2x1dGlvbnMubmV0XSANClNl
bnQ6IE1vbmRheSwgRmVicnVhcnkgMDQsIDIwMTMgODo1OCBQTQ0KVG86IEFtaXQgU0hBS1lBOyBD
aHJpc3RpYW4gTGFtcGFydGVyDQpDYzogSm9obiBXLiBMaW52aWxsZTsgbGludXgtd2lyZWxlc3MN
ClN1YmplY3Q6IFJlOiBbUEFUQ0hdIG1hYzgwMjExOiBGaXggUE4gY29ycnVwdGlvbiBpbiBjYXNl
IG9mIG11bHRpcGxlIHZpcnR1YWwgaW50ZXJmYWNlDQoNCk9uIE1vbiwgMjAxMy0wMi0wNCBhdCAx
Njo0OCArMDUzMCwgQW1pdCBTaGFreWEgd3JvdGU6DQo+IEluIGNhc2Ugb2YgbXVsdGlwbGUgdmly
dHVhbCBpbnRlcmZhY2UsIGlmIEFFUyBpcyBjb25maWd1cmVkIG9uIA0KPiBtdWx0aXBsZSBpbnRl
cmZhY2UgdGhlbiB0aGVyZSBhcmUgY2hhbmNlcyBvZiBzdG9yZWQgUE4gY29ycnVwdGlvbiwgDQo+
IGNhdXNpbmcgdHJhZmZpYyB0byBzdG9wLg0KDQpJbnRlcmVzdGluZywgbmljZSBmaW5kLg0KDQo+
IEluIGNhc2UsIGllZWU4MDIxMV9yeF9oYW5kbGVycyBwcm9jZXNzaW5nIGlzIGdvaW5nIG9uIGZv
ciBza2JzIA0KPiByZWNlaXZlZCBvbiBvbmUgdmlmIGFuZCBhdCB0aGUgc2FtZSB0aW1lLCByeCBh
Z2dyZWdhdGlvbiByZW9yZGVyIHRpbWVyIA0KPiBleHBpcmVzIG9uIGFub3RoZXIgdmlmIHRoZW4g
c3RhX3J4X2FnZ19yZW9yZGVyX3RpbWVyX2V4cGlyZWQgaXMgDQo+IGludm9rZWQgYW5kIGl0IHdp
bGwgcHVzaCBza2JzIGludG8gdGhlIHNpbmdsZSBxdWV1ZSAobG9jYWwtPnJ4X3NrYl9xdWV1ZSku
DQo+IGllZWU4MDIxMV9yeF9oYW5kbGVycyBpbiB0aGUgd2hpbGUgbG9vcCBhc3N1bWVzIHRoYXQg
dGhlIHNrYnMgYXJlIGZvciANCj4gdGhlIHNhbWUgVElEIGFuZCBzYW1lIHN0YS4NCg0KVGhpcyBp
cyBiZWNhdXNlIG9mIHN0cnVjdCBpZWVlODAyMTFfcnhfZGF0YSwgcHJlc3VtYWJseT8gSU9XLCB0
aGUgbG9vcCBkb2Vzbid0IHJlYWxseSBhc3N1bWUgaXQsIGl0J3MgdGhlIGZhY3QgdGhhdCB0aGUg
bG9vcCBpcyBjYWxsZWQgd2l0aCBhIGdpdmVuIHN0cnVjdCBpZWVlODAyMTFfcnhfZGF0YSwgcmln
aHQ/DQpbQVNdIFllcw0KDQo+IFRoaXMgYXNzdW1wdGlvbiBkb2Vzbid0IGhvbGQgZ29vZCBpbiB0
aGlzIHNjZW5hcmlvIGFuZCB0aGUgUE4gZ2V0cyANCj4gY29ycnVwdGVkIGJ5IFBOIHJlY2VpdmVk
IGluIG90aGVyIHZpZidzIHNrYiwgY2F1c2luZyB0cmFmZmljIHRvIHN0b3AgDQo+IGR1ZSB0byBQ
TiBtaXNtYXRjaC4NCj4gVGhpcyBjYW4gYmUgYXZvaWRlZCBieSBjb21wYXJpbmcgc291cmNlIG1h
YyBhZGRyZXMgaW4gcmVjZWl2ZWQgc2tiJ3MgDQo+IHdpdGggdGhlIHN0YSdzIG1hYyBhZGRyZXNz
IGZvciB3aGljaCBwcm9jZXNzaW5nIGlzIGdvaW5nIG9uLCB3aGVuIGRlLXF1ZXVlaW5nLg0KDQoN
Cg0KPiBAQCAtMjc5MCw3ICsyNzkxLDIwIEBAIHN0YXRpYyB2b2lkIGllZWU4MDIxMV9yeF9oYW5k
bGVycyhzdHJ1Y3QgDQo+IGllZWU4MDIxMV9yeF9kYXRhICpyeCkNCj4gIA0KPiAgCXJ4LT5sb2Nh
bC0+cnVubmluZ19yeF9oYW5kbGVyID0gdHJ1ZTsNCj4gIA0KPiAtCXdoaWxlICgoc2tiID0gX19z
a2JfZGVxdWV1ZSgmcngtPmxvY2FsLT5yeF9za2JfcXVldWUpKSkgew0KPiArCXNrYl9xdWV1ZV93
YWxrX3NhZmUoJnJ4LT5sb2NhbC0+cnhfc2tiX3F1ZXVlLCBza2IsIHRtcCkgew0KPiArCQlpZiAo
IXNrYikNCj4gKwkJCWJyZWFrOw0KPiArCQloZHIgPSAoc3RydWN0IGllZWU4MDIxMV9oZHIgKikg
c2tiLT5kYXRhOw0KPiArCQkvKg0KPiArCQkqIEFkZGl0aW9uYWwgY2hlY2sgdG8gZW5zdXJlIHRo
YXQgdGhlIHBhY2tldHMgY29ycmVzcG9uZGluZw0KPiArCQkqIHRvIHNhbWUgc3RhIGVudHJ5IGFz
IGluIHJ4LT5zdGEgYXJlIGRlLXF1ZXVlZC4gVGhlIHF1ZXVlDQo+ICsJCSogY2FuIGhhdmUgZGlm
ZmVyZW50IGludGVyZmFjZSBwYWNrZXRzIGluIGNhc2Ugb2YgbXVsdGlwbGUgdmlmcw0KPiArCQkq
Lw0KPiArCQlpZiAoKHJ4LT5zdGEgJiYgaGRyKSAmJiAoaWVlZTgwMjExX2lzX2RhdGEoaGRyLT5m
cmFtZV9jb250cm9sKSkNCj4gKwkJCSYmIChtZW1jbXAocngtPnN0YS0+c3RhLmFkZHIsIGhkci0+
YWRkcjIsIEVUSF9BTEVOKSkpDQo+ICsJCQkJCWNvbnRpbnVlOw0KPiArCQlfX3NrYl91bmxpbmso
c2tiLCAmcngtPmxvY2FsLT5yeF9za2JfcXVldWUpOw0KDQpBcGFydCBmcm9tIHRoZSBjdXJpb3Vz
IGNvZGluZyBzdHlsZSAoeW91IGRvbid0IG5lZWQgYW55IG9mIHRob3NlIGV4dHJhIHBhcmVudGhl
c2VzLCAmJiBzaG91bGQgYmUgb24gdGhlIHByZXZpb3VzIGxpbmUsIHRoZSBpZiBjb250aW51YXRp
b24gaW5kZW50ZWQgdG8gdGhlIG9wZW5pbmcgcGFyZW50aGVzaXMsIGNvbnRpbnVlIG9ubHkgaW5k
ZW50ZWQgb25lIHRhYiksIEkgd29uZGVyIGlmIHRoaXMgY291bGQgbGVhZCB0byBsZWFraW5nIGZy
YW1lcyBoZXJlLCBpZiB0aGUgc3RhdGlvbiBkaXNjb25uZWN0cyBvciBzb21ldGhpbmcgd2hpbGUg
dGhlcmUgYXJlIGZyYW1lcyBmb3IgaXQgb24gdGhlIHF1ZXVlPw0KSU9XLCB0aGUgImp1c3Qgc2tp
cCB0aGF0IGZyYW1lIiBwaWVjZSBzZWVtcyBhIGJpdCBxdWVzdGlvbmFibGUuDQoNCltBU10gWWVh
aCwgSSByZWFsaXplZCB0aGlzIHBhcmVudGhlc2lzL2luZGVudGF0aW9uIGlzc3VlICh3b3VsZCBm
aXggaXQpLkJUVyB3ZSBkaWQgdGVzdCB0aGlzIG91dCBhbmQgZGlkbuKAmXQgb2JzZXJ2ZSBhbnkg
c3VjaCBpc3N1ZS4gQ2FuIHlvdSBwbGVhc2UgaGVscCBtZSB1bmRlcnN0YW5kIHRoZSBmbG93IHdo
aWNoIGNvdWxkIGxlYWQgdG8gdGhlIHNhbWU/DQoNCkNocmlzdGlhbiwgaXMgdGhlcmUgYW55IHJl
YXNvbiB0byBub3QganVzdCBoYXZlIHRoZSBxdWV1ZSBiZSBvbiB0aGUgc3RhY2ssIGFuZCB1c2Ug
YSBzZXBhcmF0ZSBzcGlubG9jayBpbiB0aGUgbG9jYWwgc3RydWN0IHRvIGxvY2sgb3V0IHRoZSB1
bndhbnRlZCBjb25jdXJyZW5jeT8gSXQgc2VlbXMgdG8gbWUgdGhhdCBzaG91bGQgd29yayBqdXN0
IGFzIHdlbGwsIHNpbmNlIHRoZXJlIGFyZSBuZXZlciBmcmFtZXMgb24gdGhlIHJ4X3NrYl9xdWV1
ZSBmb3IgdmVyeSBsb25nLCByaWdodD8NCg0Kam9oYW5uZXMNCg0K

RE: [PATCH] mac80211: Fix PN corruption in case of multiple virtual interface

[Amit SHAKYA]

TXkgY29tbWVudHMgaW5saW5lW0FTXToNCg0KLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCkZy
b206IEpvaGFubmVzIEJlcmcgW21haWx0bzpqb2hhbm5lc0BzaXBzb2x1dGlvbnMubmV0XQ0KU2Vu
dDogTW9uZGF5LCBGZWJydWFyeSAwNCwgMjAxMyA4OjU4IFBNDQpUbzogQW1pdCBTSEFLWUE7IENo
cmlzdGlhbiBMYW1wYXJ0ZXINCkNjOiBKb2huIFcuIExpbnZpbGxlOyBsaW51eC13aXJlbGVzcw0K
U3ViamVjdDogUmU6IFtQQVRDSF0gbWFjODAyMTE6IEZpeCBQTiBjb3JydXB0aW9uIGluIGNhc2Ug
b2YgbXVsdGlwbGUgdmlydHVhbCBpbnRlcmZhY2UNCg0KT24gTW9uLCAyMDEzLTAyLTA0IGF0IDE2
OjQ4ICswNTMwLCBBbWl0IFNoYWt5YSB3cm90ZToNCj4gSW4gY2FzZSBvZiBtdWx0aXBsZSB2aXJ0
dWFsIGludGVyZmFjZSwgaWYgQUVTIGlzIGNvbmZpZ3VyZWQgb24gDQo+IG11bHRpcGxlIGludGVy
ZmFjZSB0aGVuIHRoZXJlIGFyZSBjaGFuY2VzIG9mIHN0b3JlZCBQTiBjb3JydXB0aW9uLCANCj4g
Y2F1c2luZyB0cmFmZmljIHRvIHN0b3AuDQoNCkludGVyZXN0aW5nLCBuaWNlIGZpbmQuDQoNCj4g
SW4gY2FzZSwgaWVlZTgwMjExX3J4X2hhbmRsZXJzIHByb2Nlc3NpbmcgaXMgZ29pbmcgb24gZm9y
IHNrYnMgDQo+IHJlY2VpdmVkIG9uIG9uZSB2aWYgYW5kIGF0IHRoZSBzYW1lIHRpbWUsIHJ4IGFn
Z3JlZ2F0aW9uIHJlb3JkZXIgdGltZXIgDQo+IGV4cGlyZXMgb24gYW5vdGhlciB2aWYgdGhlbiBz
dGFfcnhfYWdnX3Jlb3JkZXJfdGltZXJfZXhwaXJlZCBpcyANCj4gaW52b2tlZCBhbmQgaXQgd2ls
bCBwdXNoIHNrYnMgaW50byB0aGUgc2luZ2xlIHF1ZXVlIChsb2NhbC0+cnhfc2tiX3F1ZXVlKS4N
Cj4gaWVlZTgwMjExX3J4X2hhbmRsZXJzIGluIHRoZSB3aGlsZSBsb29wIGFzc3VtZXMgdGhhdCB0
aGUgc2ticyBhcmUgZm9yIA0KPiB0aGUgc2FtZSBUSUQgYW5kIHNhbWUgc3RhLg0KDQpUaGlzIGlz
IGJlY2F1c2Ugb2Ygc3RydWN0IGllZWU4MDIxMV9yeF9kYXRhLCBwcmVzdW1hYmx5PyBJT1csIHRo
ZSBsb29wIGRvZXNuJ3QgcmVhbGx5IGFzc3VtZSBpdCwgaXQncyB0aGUgZmFjdCB0aGF0IHRoZSBs
b29wIGlzIGNhbGxlZCB3aXRoIGEgZ2l2ZW4gc3RydWN0IGllZWU4MDIxMV9yeF9kYXRhLCByaWdo
dD8NCltBU10gWWVzDQoNCj4gVGhpcyBhc3N1bXB0aW9uIGRvZXNuJ3QgaG9sZCBnb29kIGluIHRo
aXMgc2NlbmFyaW8gYW5kIHRoZSBQTiBnZXRzIA0KPiBjb3JydXB0ZWQgYnkgUE4gcmVjZWl2ZWQg
aW4gb3RoZXIgdmlmJ3Mgc2tiLCBjYXVzaW5nIHRyYWZmaWMgdG8gc3RvcCANCj4gZHVlIHRvIFBO
IG1pc21hdGNoLg0KPiBUaGlzIGNhbiBiZSBhdm9pZGVkIGJ5IGNvbXBhcmluZyBzb3VyY2UgbWFj
IGFkZHJlcyBpbiByZWNlaXZlZCBza2IncyANCj4gd2l0aCB0aGUgc3RhJ3MgbWFjIGFkZHJlc3Mg
Zm9yIHdoaWNoIHByb2Nlc3NpbmcgaXMgZ29pbmcgb24sIHdoZW4gZGUtcXVldWVpbmcuDQoNCg0K
DQo+IEBAIC0yNzkwLDcgKzI3OTEsMjAgQEAgc3RhdGljIHZvaWQgaWVlZTgwMjExX3J4X2hhbmRs
ZXJzKHN0cnVjdCANCj4gaWVlZTgwMjExX3J4X2RhdGEgKnJ4KQ0KPiAgDQo+ICAJcngtPmxvY2Fs
LT5ydW5uaW5nX3J4X2hhbmRsZXIgPSB0cnVlOw0KPiAgDQo+IC0Jd2hpbGUgKChza2IgPSBfX3Nr
Yl9kZXF1ZXVlKCZyeC0+bG9jYWwtPnJ4X3NrYl9xdWV1ZSkpKSB7DQo+ICsJc2tiX3F1ZXVlX3dh
bGtfc2FmZSgmcngtPmxvY2FsLT5yeF9za2JfcXVldWUsIHNrYiwgdG1wKSB7DQo+ICsJCWlmICgh
c2tiKQ0KPiArCQkJYnJlYWs7DQo+ICsJCWhkciA9IChzdHJ1Y3QgaWVlZTgwMjExX2hkciAqKSBz
a2ItPmRhdGE7DQo+ICsJCS8qDQo+ICsJCSogQWRkaXRpb25hbCBjaGVjayB0byBlbnN1cmUgdGhh
dCB0aGUgcGFja2V0cyBjb3JyZXNwb25kaW5nDQo+ICsJCSogdG8gc2FtZSBzdGEgZW50cnkgYXMg
aW4gcngtPnN0YSBhcmUgZGUtcXVldWVkLiBUaGUgcXVldWUNCj4gKwkJKiBjYW4gaGF2ZSBkaWZm
ZXJlbnQgaW50ZXJmYWNlIHBhY2tldHMgaW4gY2FzZSBvZiBtdWx0aXBsZSB2aWZzDQo+ICsJCSov
DQo+ICsJCWlmICgocngtPnN0YSAmJiBoZHIpICYmIChpZWVlODAyMTFfaXNfZGF0YShoZHItPmZy
YW1lX2NvbnRyb2wpKQ0KPiArCQkJJiYgKG1lbWNtcChyeC0+c3RhLT5zdGEuYWRkciwgaGRyLT5h
ZGRyMiwgRVRIX0FMRU4pKSkNCj4gKwkJCQkJY29udGludWU7DQo+ICsJCV9fc2tiX3VubGluayhz
a2IsICZyeC0+bG9jYWwtPnJ4X3NrYl9xdWV1ZSk7DQoNCkFwYXJ0IGZyb20gdGhlIGN1cmlvdXMg
Y29kaW5nIHN0eWxlICh5b3UgZG9uJ3QgbmVlZCBhbnkgb2YgdGhvc2UgZXh0cmEgcGFyZW50aGVz
ZXMsICYmIHNob3VsZCBiZSBvbiB0aGUgcHJldmlvdXMgbGluZSwgdGhlIGlmIGNvbnRpbnVhdGlv
biBpbmRlbnRlZCB0byB0aGUgb3BlbmluZyBwYXJlbnRoZXNpcywgY29udGludWUgb25seSBpbmRl
bnRlZCBvbmUgdGFiKSwgSSB3b25kZXIgaWYgdGhpcyBjb3VsZCBsZWFkIHRvIGxlYWtpbmcgZnJh
bWVzIGhlcmUsIGlmIHRoZSBzdGF0aW9uIGRpc2Nvbm5lY3RzIG9yIHNvbWV0aGluZyB3aGlsZSB0
aGVyZSBhcmUgZnJhbWVzIGZvciBpdCBvbiB0aGUgcXVldWU/DQpJT1csIHRoZSAianVzdCBza2lw
IHRoYXQgZnJhbWUiIHBpZWNlIHNlZW1zIGEgYml0IHF1ZXN0aW9uYWJsZS4NCg0KW0FTXSBZZWFo
LCBJIHJlYWxpemVkIHRoaXMgcGFyZW50aGVzaXMvaW5kZW50YXRpb24gaXNzdWUgKHdvdWxkIGZp
eCBpdCkuQlRXIHdlIGRpZCB0ZXN0IHRoaXMgb3V0IGFuZCBkaWRu4oCZdCBvYnNlcnZlIGFueSBz
dWNoIGlzc3VlLiBDYW4geW91IHBsZWFzZSBoZWxwIG1lIHVuZGVyc3RhbmQgdGhlIGZsb3cgd2hp
Y2ggY291bGQgbGVhZCB0byB0aGUgc2FtZT8gQWxzbyBpbiBjYXNlIHRoaXMgaXMgYW4gaXNzdWUs
IGNhbiB3ZSB0YWtlIGNhcmUgb2YgdGhpcyBpbiB0aGUgY2xlYW51cCByZWxhdGVkIHRvIGRpc2Nv
bm5lY3Q/IEhlcmUgaXQgc2VlbXMgYSBjb25zY2lvdXMgZWZmb3J0IGhhcyBiZWVuIG1hZGUgdG8g
YXZvaWQgc3BpbmxvY2sgKHJ4LT5sb2NhbC0+cnhfc2tiX3F1ZXVlLmxvY2spLCBhcyB0aGlzIGxv
Y2sgaXMgdGFrZW4gb25seSBmb3IgdGhlIGR1cmF0aW9uIG9mIGRlcXVldWUuIFRoZSBzdWdnZXN0
ZWQgc29sdXRpb24gYXZvaWRzIHVzaW5nIHNwaW5sb2NrLg0KDQpDaHJpc3RpYW4sIGlzIHRoZXJl
IGFueSByZWFzb24gdG8gbm90IGp1c3QgaGF2ZSB0aGUgcXVldWUgYmUgb24gdGhlIHN0YWNrLCBh
bmQgdXNlIGEgc2VwYXJhdGUgc3BpbmxvY2sgaW4gdGhlIGxvY2FsIHN0cnVjdCB0byBsb2NrIG91
dCB0aGUgdW53YW50ZWQgY29uY3VycmVuY3k/IEl0IHNlZW1zIHRvIG1lIHRoYXQgc2hvdWxkIHdv
cmsganVzdCBhcyB3ZWxsLCBzaW5jZSB0aGVyZSBhcmUgbmV2ZXIgZnJhbWVzIG9uIHRoZSByeF9z
a2JfcXVldWUgZm9yIHZlcnkgbG9uZywgcmlnaHQ/DQoNCmpvaGFubmVzDQoNCg==

Re: [PATCH] mac80211: Fix PN corruption in case of multiple virtual interface

[Christian Lamparter]

On Wednesday, February 06, 2013 07:56:46 AM Amit SHAKYA wrote:
> From: Johannes Berg [mailto:johannes@sipsolutions.net]
> On Mon, 2013-02-04 at 16:48 +0530, Amit Shakya wrote:
> > @@ -2790,7 +2791,20 @@ static void ieee80211_rx_handlers(struct 
> > ieee80211_rx_data *rx)
> >  
> >  	rx->local->running_rx_handler = true;
> >  
> > -	while ((skb = __skb_dequeue(&rx->local->rx_skb_queue))) {
> > +	skb_queue_walk_safe(&rx->local->rx_skb_queue, skb, tmp) {
> > +		if (!skb)
> > +			break;
> > +		hdr = (struct ieee80211_hdr *) skb->data;
> > +		/*
> > +		* Additional check to ensure that the packets corresponding
> > +		* to same sta entry as in rx->sta are de-queued. The queue
> > +		* can have different interface packets in case of multiple vifs
> > +		*/
> > +		if ((rx->sta && hdr) && (ieee80211_is_data(hdr->frame_control))
> > +			&& (memcmp(rx->sta->sta.addr, hdr->addr2, ETH_ALEN)))
> > +					continue;
> > +		__skb_unlink(skb, &rx->local->rx_skb_queue);

> I wonder if this could lead to leaking frames here, if the station 
> disconnects or something while there are frames for it on the queue?
> IOW, the "just skip that frame" piece seems a bit questionable.
> 
>[AS] BTW we did test this out and didn’t observe any such issue. Can you
>     please help me understand the flow which could lead to the same? 
I read it like this: If a station suddenly disappears (for good) while
it still has some data in the reorder buffer, the reorder release timer
will put these orphaned frames into rx_skb_queue. 
With this patch, they will never be cleared from the queue, until
ieee80211_unregister_hw is called [when the device is unregistered].

So, you would need to go through the rx_skb_queue everytime a HT 
station is torn down and remove the affected frames from there.

>     Also in case this is an issue, can we take care of this in the cleanup
>     related to disconnect?
Sure, you could do that in ieee80211_sta_tear_down_BA_sessions. But you
don't need to. On Monday, I posted a patch:
<http://www.spinics.net/lists/linux-wireless/msg102725.html>
it should take care of the issue. So, can you test it please? 

>     Here it seems a conscious effort has been made to avoid spinlock
>     (rx->local->rx_skb_queue.lock), as this lock is taken only for the
>     duration of dequeue. The suggested solution avoids using spinlock.
Oh no, the locking is there. skb_unlink is defined in net/core/skbuff.c 
as a spin_lock wrapped __skb_unlink. The same is true for skb_queue_tail
and __skb_queue_tail. (Or are you talking about something else?)

Regards

Christian

RE: [PATCH] mac80211: Fix PN corruption in case of multiple virtual interface

[Amit SHAKYA]

SGkgQ2hyaXN0aWFuLA0KDQpBY3R1YWxseSBJIGRlYnVnZ2VkIHRoaXMgaXNzdWUgcXVpdGUgc29t
ZSB0aW1lIGJhY2sgZm9yIGEgY3VzdG9tZXIgcHJvamVjdC4gTm93IEkgZG9u4oCZdCBoYXZlIHRo
ZSBzZXR1cCBhbHNvIGFzIEkgaGF2ZSBtb3ZlZCBvdXQgb2YgdGhlIHByb2plY3QuDQoNClNvIEkg
YW0gYWZyYWlkIEkgd2lsbCBub3QgYmUgYWJsZSB0byB0ZXN0IGl0Lg0KDQpSZWdhcmRzDQpBbWl0
DQoNCi0tLS0tT3JpZ2luYWwgTWVzc2FnZS0tLS0tDQpGcm9tOiBDaHJpc3RpYW4gTGFtcGFydGVy
IFttYWlsdG86Y2h1bmtlZXlAZ29vZ2xlbWFpbC5jb21dIA0KU2VudDogV2VkbmVzZGF5LCBGZWJy
dWFyeSAwNiwgMjAxMyA3OjAzIFBNDQpUbzogQW1pdCBTSEFLWUENCkNjOiBKb2hhbm5lcyBCZXJn
OyBKb2huIFcuIExpbnZpbGxlOyBsaW51eC13aXJlbGVzcw0KU3ViamVjdDogUmU6IFtQQVRDSF0g
bWFjODAyMTE6IEZpeCBQTiBjb3JydXB0aW9uIGluIGNhc2Ugb2YgbXVsdGlwbGUgdmlydHVhbCBp
bnRlcmZhY2UNCg0KT24gV2VkbmVzZGF5LCBGZWJydWFyeSAwNiwgMjAxMyAwNzo1Njo0NiBBTSBB
bWl0IFNIQUtZQSB3cm90ZToNCj4gRnJvbTogSm9oYW5uZXMgQmVyZyBbbWFpbHRvOmpvaGFubmVz
QHNpcHNvbHV0aW9ucy5uZXRdDQo+IE9uIE1vbiwgMjAxMy0wMi0wNCBhdCAxNjo0OCArMDUzMCwg
QW1pdCBTaGFreWEgd3JvdGU6DQo+ID4gQEAgLTI3OTAsNyArMjc5MSwyMCBAQCBzdGF0aWMgdm9p
ZCBpZWVlODAyMTFfcnhfaGFuZGxlcnMoc3RydWN0IA0KPiA+IGllZWU4MDIxMV9yeF9kYXRhICpy
eCkNCj4gPiAgDQo+ID4gIAlyeC0+bG9jYWwtPnJ1bm5pbmdfcnhfaGFuZGxlciA9IHRydWU7DQo+
ID4gIA0KPiA+IC0Jd2hpbGUgKChza2IgPSBfX3NrYl9kZXF1ZXVlKCZyeC0+bG9jYWwtPnJ4X3Nr
Yl9xdWV1ZSkpKSB7DQo+ID4gKwlza2JfcXVldWVfd2Fsa19zYWZlKCZyeC0+bG9jYWwtPnJ4X3Nr
Yl9xdWV1ZSwgc2tiLCB0bXApIHsNCj4gPiArCQlpZiAoIXNrYikNCj4gPiArCQkJYnJlYWs7DQo+
ID4gKwkJaGRyID0gKHN0cnVjdCBpZWVlODAyMTFfaGRyICopIHNrYi0+ZGF0YTsNCj4gPiArCQkv
Kg0KPiA+ICsJCSogQWRkaXRpb25hbCBjaGVjayB0byBlbnN1cmUgdGhhdCB0aGUgcGFja2V0cyBj
b3JyZXNwb25kaW5nDQo+ID4gKwkJKiB0byBzYW1lIHN0YSBlbnRyeSBhcyBpbiByeC0+c3RhIGFy
ZSBkZS1xdWV1ZWQuIFRoZSBxdWV1ZQ0KPiA+ICsJCSogY2FuIGhhdmUgZGlmZmVyZW50IGludGVy
ZmFjZSBwYWNrZXRzIGluIGNhc2Ugb2YgbXVsdGlwbGUgdmlmcw0KPiA+ICsJCSovDQo+ID4gKwkJ
aWYgKChyeC0+c3RhICYmIGhkcikgJiYgKGllZWU4MDIxMV9pc19kYXRhKGhkci0+ZnJhbWVfY29u
dHJvbCkpDQo+ID4gKwkJCSYmIChtZW1jbXAocngtPnN0YS0+c3RhLmFkZHIsIGhkci0+YWRkcjIs
IEVUSF9BTEVOKSkpDQo+ID4gKwkJCQkJY29udGludWU7DQo+ID4gKwkJX19za2JfdW5saW5rKHNr
YiwgJnJ4LT5sb2NhbC0+cnhfc2tiX3F1ZXVlKTsNCg0KPiBJIHdvbmRlciBpZiB0aGlzIGNvdWxk
IGxlYWQgdG8gbGVha2luZyBmcmFtZXMgaGVyZSwgaWYgdGhlIHN0YXRpb24gDQo+IGRpc2Nvbm5l
Y3RzIG9yIHNvbWV0aGluZyB3aGlsZSB0aGVyZSBhcmUgZnJhbWVzIGZvciBpdCBvbiB0aGUgcXVl
dWU/DQo+IElPVywgdGhlICJqdXN0IHNraXAgdGhhdCBmcmFtZSIgcGllY2Ugc2VlbXMgYSBiaXQg
cXVlc3Rpb25hYmxlLg0KPiANCj5bQVNdIEJUVyB3ZSBkaWQgdGVzdCB0aGlzIG91dCBhbmQgZGlk
buKAmXQgb2JzZXJ2ZSBhbnkgc3VjaCBpc3N1ZS4gQ2FuIHlvdQ0KPiAgICAgcGxlYXNlIGhlbHAg
bWUgdW5kZXJzdGFuZCB0aGUgZmxvdyB3aGljaCBjb3VsZCBsZWFkIHRvIHRoZSBzYW1lPyANCkkg
cmVhZCBpdCBsaWtlIHRoaXM6IElmIGEgc3RhdGlvbiBzdWRkZW5seSBkaXNhcHBlYXJzIChmb3Ig
Z29vZCkgd2hpbGUgaXQgc3RpbGwgaGFzIHNvbWUgZGF0YSBpbiB0aGUgcmVvcmRlciBidWZmZXIs
IHRoZSByZW9yZGVyIHJlbGVhc2UgdGltZXIgd2lsbCBwdXQgdGhlc2Ugb3JwaGFuZWQgZnJhbWVz
IGludG8gcnhfc2tiX3F1ZXVlLiANCldpdGggdGhpcyBwYXRjaCwgdGhleSB3aWxsIG5ldmVyIGJl
IGNsZWFyZWQgZnJvbSB0aGUgcXVldWUsIHVudGlsIGllZWU4MDIxMV91bnJlZ2lzdGVyX2h3IGlz
IGNhbGxlZCBbd2hlbiB0aGUgZGV2aWNlIGlzIHVucmVnaXN0ZXJlZF0uDQoNClNvLCB5b3Ugd291
bGQgbmVlZCB0byBnbyB0aHJvdWdoIHRoZSByeF9za2JfcXVldWUgZXZlcnl0aW1lIGEgSFQgc3Rh
dGlvbiBpcyB0b3JuIGRvd24gYW5kIHJlbW92ZSB0aGUgYWZmZWN0ZWQgZnJhbWVzIGZyb20gdGhl
cmUuDQoNCj4gICAgIEFsc28gaW4gY2FzZSB0aGlzIGlzIGFuIGlzc3VlLCBjYW4gd2UgdGFrZSBj
YXJlIG9mIHRoaXMgaW4gdGhlIGNsZWFudXANCj4gICAgIHJlbGF0ZWQgdG8gZGlzY29ubmVjdD8N
ClN1cmUsIHlvdSBjb3VsZCBkbyB0aGF0IGluIGllZWU4MDIxMV9zdGFfdGVhcl9kb3duX0JBX3Nl
c3Npb25zLiBCdXQgeW91IGRvbid0IG5lZWQgdG8uIE9uIE1vbmRheSwgSSBwb3N0ZWQgYSBwYXRj
aDoNCjxodHRwOi8vd3d3LnNwaW5pY3MubmV0L2xpc3RzL2xpbnV4LXdpcmVsZXNzL21zZzEwMjcy
NS5odG1sPg0KaXQgc2hvdWxkIHRha2UgY2FyZSBvZiB0aGUgaXNzdWUuIFNvLCBjYW4geW91IHRl
c3QgaXQgcGxlYXNlPyANCg0KPiAgICAgSGVyZSBpdCBzZWVtcyBhIGNvbnNjaW91cyBlZmZvcnQg
aGFzIGJlZW4gbWFkZSB0byBhdm9pZCBzcGlubG9jaw0KPiAgICAgKHJ4LT5sb2NhbC0+cnhfc2ti
X3F1ZXVlLmxvY2spLCBhcyB0aGlzIGxvY2sgaXMgdGFrZW4gb25seSBmb3IgdGhlDQo+ICAgICBk
dXJhdGlvbiBvZiBkZXF1ZXVlLiBUaGUgc3VnZ2VzdGVkIHNvbHV0aW9uIGF2b2lkcyB1c2luZyBz
cGlubG9jay4NCk9oIG5vLCB0aGUgbG9ja2luZyBpcyB0aGVyZS4gc2tiX3VubGluayBpcyBkZWZp
bmVkIGluIG5ldC9jb3JlL3NrYnVmZi5jIGFzIGEgc3Bpbl9sb2NrIHdyYXBwZWQgX19za2JfdW5s
aW5rLiBUaGUgc2FtZSBpcyB0cnVlIGZvciBza2JfcXVldWVfdGFpbCBhbmQgX19za2JfcXVldWVf
dGFpbC4gKE9yIGFyZSB5b3UgdGFsa2luZyBhYm91dCBzb21ldGhpbmcgZWxzZT8pDQoNClJlZ2Fy
ZHMNCg0KQ2hyaXN0aWFuDQo=

Re: [PATCH] mac80211: Fix PN corruption in case of multiple virtual interface

[Johannes Berg]

Hi Amit,


> Actually I debugged this issue quite some time back for a customer project. Now I don’t have the setup also as I have moved out of the project.
> 
> So I am afraid I will not be able to test it.

Ah, too bad.

Christian, I think regardless of that your version is much better, can
you send it?

Ben, maybe you've seen this as well, with your many interfaces
testing? :-)

johannes

Re: [PATCH] mac80211: Fix PN corruption in case of multiple virtual interface

[Ben Greear]

On 02/08/2013 12:50 AM, Johannes Berg wrote:
> Hi Amit,
>
>
>> Actually I debugged this issue quite some time back for a customer project. Now I don’t have the setup also as I have moved out of the project.
>>
>> So I am afraid I will not be able to test it.
>
> Ah, too bad.
>
> Christian, I think regardless of that your version is much better, can
> you send it?
>
> Ben, maybe you've seen this as well, with your many interfaces
> testing? :-)

Well, I know (total) throughput goes to shit when I add 100+ interfaces, but I'm
not sure why..I assumed it was mainly because the MPDU logic didn't work
well when there are lots of slow traffic flows and haven't made any real
attempt to figure out if that is true or not.

I'm feverishly trying to get a stable 3.7 out the door, but as soon
as that goes, I'll start some testing on whatever is the latest upstream
code again, and run some more detailed tests on throughput with lots of
interfaces...

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH] mac80211: protect rx-path with spinlock

[Johannes Berg]

On Mon, 2013-02-04 at 17:44 +0000, Christian Lamparter wrote:
> This patch fixes the problem which was discussed in
> "mac80211: Fix PN corruption in case of multiple
> virtual interface" [1].
> 
> Amit Shakya reported a serious issue with my patch:
> mac80211: serialize rx path workers" [2]:
> 
> In case, ieee80211_rx_handlers processing is going on
> for skbs received on one vif and at the same time, rx
> aggregation reorder timer expires on another vif then
> sta_rx_agg_reorder_timer_expired is invoked and it will
> push skbs into the single queue (local->rx_skb_queue).
> 
> ieee80211_rx_handlers in the while loop assumes that
> the skbs are for the same sdata and sta. This assumption
> doesn't hold good in this scenario and the PN gets
> corrupted by PN received in other vif's skb, causing
> traffic to stop due to PN mismatch."

Applied. It's kinda late in the merge window, so I'm not pushing it for
3.8 any more, since very few people seem to have noticed this issue.

If needed, somebody else can champion it for stable :-)

johannes

Re: [PATCH] mac80211: protect rx-path with spinlock

[Christian Lamparter]

On Friday 08 February 2013 22:36:17 Johannes Berg wrote:
> On Mon, 2013-02-04 at 17:44 +0000, Christian Lamparter wrote:
> > This patch fixes the problem which was discussed in
> > "mac80211: Fix PN corruption in case of multiple
> > virtual interface" [1].
> > 
> > Amit Shakya reported a serious issue with my patch:
> > mac80211: serialize rx path workers" [2]:
> > 
> > In case, ieee80211_rx_handlers processing is going on
> > for skbs received on one vif and at the same time, rx
> > aggregation reorder timer expires on another vif then
> > sta_rx_agg_reorder_timer_expired is invoked and it will
> > push skbs into the single queue (local->rx_skb_queue).
> > 
> > ieee80211_rx_handlers in the while loop assumes that
> > the skbs are for the same sdata and sta. This assumption
> > doesn't hold good in this scenario and the PN gets
> > corrupted by PN received in other vif's skb, causing
> > traffic to stop due to PN mismatch."
> 
> Applied. It's kinda late in the merge window, so I'm not
> pushing it for 3.8 any more, since very few people seem
> to have noticed this issue.
> 
> If needed, somebody else can champion it for stable :-)
That's most likely because BARs should actually take care
of releasing stuck frames anyway. The release timer is there
just in case the BAR which would release the ampdu reorder
buffer is also lost.

Regards,
	Christian