| Message ID | 1541507440-21008-1-git-send-email-emmanuel.grumbach@intel.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | [RFC] mac80211: fix deauth TX when we disconnect | expand |
On Tue, Nov 6, 2018 at 2:32 PM Emmanuel Grumbach <emmanuel.grumbach@intel.com> wrote: > > The iTXQs stop/wake queue mechanism involves a whole bunch > of locks and this is probably why the call to > ieee80211_wake_txqs is deferred to a tasklet when called from > __ieee80211_wake_queue. > > Another advantage of that is that ieee80211_wake_txqs might > call the wake_tx_queue() callback and then the driver may > call mac80211 which will call it back in the same context. > > The bug I saw is that when we send a deauth frame as a > station we do: > > flush(drop=1) > tx deauth > flush(drop=0) > > While we flush we stop the queues and wake them up > immediately after we finished flushing. The problem here is > that the tasklet that de-facto enables the queue may not have > run until we send the deauth. Then the deauth frame is sent > to the driver (which is surprising by itself), but the driver > won't get anything useful from ieee80211_tx_dequeue because > the queue is stopped (or more precisely because > vif->txqs_stopped[0] is true). > Then the deauth is not sent. Later on, the tasklet will run, > but that'll be too late. We'll already have removed all the > vif etc... > > There are 2 options I see here: > 1) do what I did in this patch (which is pretty awful because > of the if (lock) thing. If we wake up the queue because of > a reason which is internal to mac80211, call > ieee80211_wake_txqs synchronously since we know we are not > coming from the driver. > 2) we could set vif->txqs_stopped to false synchrously and do > the rest of the job in a tasklet. This would allow the code > that sends the Tx to put the frame in the queue and the > driver would actually get it. > > Maybe others have better ideas? > > Change-Id: I5d3bd20ec765becb91944741c35e35f6e3888981 > Cc: Manikanta Pubbisetty <mpubbise@codeaurora.org> > Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com> > --- > net/mac80211/util.c | 24 ++++++++++++++++++------ > 1 file changed, 18 insertions(+), 6 deletions(-) > > diff --git a/net/mac80211/util.c b/net/mac80211/util.c > index c9cb00a..c731ddb 100644 > --- a/net/mac80211/util.c > +++ b/net/mac80211/util.c > @@ -299,16 +299,16 @@ out: > spin_unlock_bh(&fq->lock); > } > > -void ieee80211_wake_txqs(unsigned long data) > +static void _ieee80211_wake_txqs(struct ieee80211_local *local, bool lock) > { > - struct ieee80211_local *local = (struct ieee80211_local *)data; > struct ieee80211_sub_if_data *sdata; > int n_acs = IEEE80211_NUM_ACS; > unsigned long flags; > int i; > > rcu_read_lock(); > - spin_lock_irqsave(&local->queue_stop_reason_lock, flags); > + if (lock) > + spin_lock_irqsave(&local->queue_stop_reason_lock, flags); > This is buggy of course... If lock is false, then we'll call spin_unlock_irqrestore further down in the function, but flags won't be initialized. I don't really know why we have this strange pattern of a lock loop unlock wake_tx_queue() lock unlock If we want not to call wake_tx_queue with the lock taken, we can prepare a bitmap of ACs and then call wake_tx_queue() for all the ACs, bu then of course, we may have races. OTOH, we may have races here as well. Someone could change queue_stop_reasons after we release the lock and before we call wake_tx_queue and because of that, we'd end up waking a queue that is supposed to be stopped. > if (local->hw.queues < IEEE80211_NUM_ACS) > n_acs = 1; > @@ -332,10 +332,18 @@ void ieee80211_wake_txqs(unsigned long data) > spin_lock_irqsave(&local->queue_stop_reason_lock, flags); > } > > - spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags); > + if (lock) > + spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags); > rcu_read_unlock(); > } > > +void ieee80211_wake_txqs(unsigned long data) > +{ > + struct ieee80211_local *local = (struct ieee80211_local *)data; > + > + _ieee80211_wake_txqs(local, true); > +} > + > void ieee80211_propagate_queue_wake(struct ieee80211_local *local, int queue) > { > struct ieee80211_sub_if_data *sdata; > @@ -405,8 +413,12 @@ static void __ieee80211_wake_queue(struct ieee80211_hw *hw, int queue, > } else > tasklet_schedule(&local->tx_pending_tasklet); > > - if (local->ops->wake_tx_queue) > - tasklet_schedule(&local->wake_txqs_tasklet); > + if (local->ops->wake_tx_queue) { > + if (reason == IEEE80211_QUEUE_STOP_REASON_DRIVER) > + tasklet_schedule(&local->wake_txqs_tasklet); > + else > + _ieee80211_wake_txqs(local, false); > + } > } > > void ieee80211_wake_queue_by_reason(struct ieee80211_hw *hw, int queue, > -- > 2.7.4 >
diff --git a/net/mac80211/util.c b/net/mac80211/util.c index c9cb00a..c731ddb 100644 --- a/net/mac80211/util.c +++ b/net/mac80211/util.c @@ -299,16 +299,16 @@ out: spin_unlock_bh(&fq->lock); } -void ieee80211_wake_txqs(unsigned long data) +static void _ieee80211_wake_txqs(struct ieee80211_local *local, bool lock) { - struct ieee80211_local *local = (struct ieee80211_local *)data; struct ieee80211_sub_if_data *sdata; int n_acs = IEEE80211_NUM_ACS; unsigned long flags; int i; rcu_read_lock(); - spin_lock_irqsave(&local->queue_stop_reason_lock, flags); + if (lock) + spin_lock_irqsave(&local->queue_stop_reason_lock, flags); if (local->hw.queues < IEEE80211_NUM_ACS) n_acs = 1; @@ -332,10 +332,18 @@ void ieee80211_wake_txqs(unsigned long data) spin_lock_irqsave(&local->queue_stop_reason_lock, flags); } - spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags); + if (lock) + spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags); rcu_read_unlock(); } +void ieee80211_wake_txqs(unsigned long data) +{ + struct ieee80211_local *local = (struct ieee80211_local *)data; + + _ieee80211_wake_txqs(local, true); +} + void ieee80211_propagate_queue_wake(struct ieee80211_local *local, int queue) { struct ieee80211_sub_if_data *sdata; @@ -405,8 +413,12 @@ static void __ieee80211_wake_queue(struct ieee80211_hw *hw, int queue, } else tasklet_schedule(&local->tx_pending_tasklet); - if (local->ops->wake_tx_queue) - tasklet_schedule(&local->wake_txqs_tasklet); + if (local->ops->wake_tx_queue) { + if (reason == IEEE80211_QUEUE_STOP_REASON_DRIVER) + tasklet_schedule(&local->wake_txqs_tasklet); + else + _ieee80211_wake_txqs(local, false); + } } void ieee80211_wake_queue_by_reason(struct ieee80211_hw *hw, int queue,
The iTXQs stop/wake queue mechanism involves a whole bunch of locks and this is probably why the call to ieee80211_wake_txqs is deferred to a tasklet when called from __ieee80211_wake_queue. Another advantage of that is that ieee80211_wake_txqs might call the wake_tx_queue() callback and then the driver may call mac80211 which will call it back in the same context. The bug I saw is that when we send a deauth frame as a station we do: flush(drop=1) tx deauth flush(drop=0) While we flush we stop the queues and wake them up immediately after we finished flushing. The problem here is that the tasklet that de-facto enables the queue may not have run until we send the deauth. Then the deauth frame is sent to the driver (which is surprising by itself), but the driver won't get anything useful from ieee80211_tx_dequeue because the queue is stopped (or more precisely because vif->txqs_stopped[0] is true). Then the deauth is not sent. Later on, the tasklet will run, but that'll be too late. We'll already have removed all the vif etc... There are 2 options I see here: 1) do what I did in this patch (which is pretty awful because of the if (lock) thing. If we wake up the queue because of a reason which is internal to mac80211, call ieee80211_wake_txqs synchronously since we know we are not coming from the driver. 2) we could set vif->txqs_stopped to false synchrously and do the rest of the job in a tasklet. This would allow the code that sends the Tx to put the frame in the queue and the driver would actually get it. Maybe others have better ideas? Change-Id: I5d3bd20ec765becb91944741c35e35f6e3888981 Cc: Manikanta Pubbisetty <mpubbise@codeaurora.org> Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com> --- net/mac80211/util.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-)