mac80211: Fix PN corruption in case of multiple virtual interface

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.

---

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

mac80211: Fix PN corruption in case of multiple virtual interface

Commit Message

Comments

Patch