DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org C2B2620869
MIME-Version: 1.0
References: <1541507440-21008-1-git-send-email-emmanuel.grumbach@intel.com>
In-Reply-To: <1541507440-21008-1-git-send-email-emmanuel.grumbach@intel.com>
From:   Emmanuel Grumbach <egrumbach@gmail.com>
Date:   Tue, 6 Nov 2018 15:19:18 +0200
Message-ID: <CANUX_P3YHNnr3=E9QjWeB6j-NL+LkbohMj1A3TS6V+g0cXUzaQ@mail.gmail.com>
Subject: Re: [RFC] mac80211: fix deauth TX when we disconnect
To:     Emmanuel Grumbach <emmanuel.grumbach@intel.com>
Cc:     linux-wireless <linux-wireless@vger.kernel.org>,
        mpubbise@codeaurora.org
Content-Type: text/plain; charset="UTF-8"
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

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
>