Re: [PATCH 3.14] mac80211: fix AP powersave TX vs. wakeup race

[Johannes Berg <johannes@sipsolutions.net>]

On Tue, 2014-06-03 at 19:05 -0700, Luis R. Rodriguez wrote:

> > +++ b/net/mac80211/tx.c
> > @@ -478,6 +478,19 @@ ieee80211_tx_h_unicast_ps_buf(struct ieee80211_tx_data *tx)
> >                        sta->sta.addr, sta->sta.aid, ac);
> >                 if (tx->local->total_ps_buffered >= TOTAL_MAX_TX_BUFFER)
> >                         purge_old_ps_buffers(tx->local);
> > +
> > +               /* sync with ieee80211_sta_ps_deliver_wakeup */
> > +               spin_lock(&sta->ps_lock);
> > +               /*
> > +                * STA woke up the meantime and all the frames on ps_tx_buf have
> > +                * been queued to pending queue. No reordering can happen, go
> > +                * ahead and Tx the packet.
> > +                */
> > +               if (!test_sta_flag(sta, WLAN_STA_PS_STA)) {
> > +                       spin_unlock(&sta->ps_lock);
> > +                       return TX_CONTINUE;
> > +               }
> > +
> >                 if (skb_queue_len(&sta->ps_tx_buf[ac]) >= STA_MAX_TX_BUFFER) {
> >                         struct sk_buff *old = skb_dequeue(&sta->ps_tx_buf[ac]);
> >                         ps_dbg(tx->sdata,
> 
> This fixes the race for the case where the API introduced via commit
> af81858172cc but that code also *moved* code, the API added simply
> added a new way to let driver to hit a trigger to send buffered
> frames, are we sure that through mechanisms that don't use this API
> this race doesn't exist? That is can we not race between
> ieee80211_rx_h_sta_process() and ieee80211_tx_h_unicast_ps_buf()?

Err, I have no idea what you're trying to say.

Did you mean sta_ps_end() vs. ieee80211_tx_h_unicast_ps_buf()? Those can
*race*, but any outcome will be valid, afaict, given the station flag
tricks. It all goes back to the same (locked) code for manipulating the
queues.

I recently fixed a bug where mac80211 would get into a confused state
and not buffer packets when it would really be required, but that's just
a bug that can be observed over the air, it has no bad consequences
except for that station losing traffic.

johannes