[RFC] mac80211: fix deauth TX when we disconnect
diff mbox series

Message ID 1541507440-21008-1-git-send-email-emmanuel.grumbach@intel.com
State RFC
Delegated to: Johannes Berg
Headers show
Series
  • [RFC] mac80211: fix deauth TX when we disconnect
Related show

Commit Message

Emmanuel Grumbach Nov. 6, 2018, 12:30 p.m. UTC
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(-)

Comments

Emmanuel Grumbach Nov. 6, 2018, 1:19 p.m. UTC | #1
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
>

Patch
diff mbox series

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,