[PATCH 1/2] mac80211: fix txq queue related crashes

The driver can access the queue simultanously
while mac80211 tears down the interface. Without
spinlock protection this could lead to corrupting
sk_buff_head and subsequently to an invalid
pointer dereference.

Fixes: ba8c3d6f16a1 ("mac80211: add an intermediate software queue implementation")
Signed-off-by: Michal Kazior <redacted>
---
 net/mac80211/iface.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 33ae3c81bfc5..0451f120746e 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c

@@ -977,7 +977,10 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
 	if (sdata->vif.txq) {
 		struct txq_info *txqi = to_txq_info(sdata->vif.txq);
 
+		spin_lock_bh(&txqi->queue.lock);
 		ieee80211_purge_tx_queue(&local->hw, &txqi->queue);
+		spin_unlock_bh(&txqi->queue.lock);
+
 		atomic_set(&sdata->txqs_len[txqi->txq.ac], 0);
 	}
 

-- 
2.1.4

This will allow drivers to make more educated
decisions whether to defer transmission or not.

Relying on wake_tx_queue() call count implicitly
was not possible because it could be called
without queued frame count actually changing on
software tx aggregation start/stop code paths.

It was also not possible to know how long
byte-wise queue was without dequeueing.

Signed-off-by: Michal Kazior <redacted>
---
 include/net/mac80211.h  |  4 ++++
 net/mac80211/iface.c    |  2 ++
 net/mac80211/sta_info.c |  2 ++
 net/mac80211/tx.c       | 11 ++++++++++-
 4 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 566df20dc957..c29ca8be9ac2 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h

@@ -1781,6 +1781,8 @@ struct ieee80211_tx_control {
  *
  * @vif: &struct ieee80211_vif pointer from the add_interface callback.
  * @sta: station table entry, %NULL for per-vif queue
+ * @qdepth: number of pending frames
+ * @qsize: number of pending bytes
  * @tid: the TID for this queue (unused for per-vif queue)
  * @ac: the AC for this queue
  * @drv_priv: driver private area, sized by hw->txq_data_size

@@ -1791,6 +1793,8 @@ struct ieee80211_tx_control {
 struct ieee80211_txq {
 	struct ieee80211_vif *vif;
 	struct ieee80211_sta *sta;
+	int qdepth;
+	int qsize;
 	u8 tid;
 	u8 ac;
 

diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 0451f120746e..dfcb19080eb0 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c

@@ -979,6 +979,8 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
 
 		spin_lock_bh(&txqi->queue.lock);
 		ieee80211_purge_tx_queue(&local->hw, &txqi->queue);
+		txqi->txq.qdepth = 0;
+		txqi->txq.qsize = 0;
 		spin_unlock_bh(&txqi->queue.lock);
 
 		atomic_set(&sdata->txqs_len[txqi->txq.ac], 0);

diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index 7e007cf12cb2..4b93a11f4a0d 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c

@@ -116,6 +116,8 @@ static void __cleanup_single_sta(struct sta_info *sta)
 
 			ieee80211_purge_tx_queue(&local->hw, &txqi->queue);
 			atomic_sub(n, &sdata->txqs_len[txqi->txq.ac]);
+			txqi->txq.qdepth = 0;
+			txqi->txq.qsize = 0;
 		}
 	}
 

diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 3311ce0f3d6c..6f9a0db3824e 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c

@@ -1266,7 +1266,13 @@ static void ieee80211_drv_tx(struct ieee80211_local *local,
 	if (atomic_read(&sdata->txqs_len[ac]) >= local->hw.txq_ac_max_pending)
 		netif_stop_subqueue(sdata->dev, ac);
 
-	skb_queue_tail(&txqi->queue, skb);
+	spin_lock_bh(&txqi->queue.lock);
+	txq->qdepth++;
+	txq->qsize += skb->len;
+
+	__skb_queue_tail(&txqi->queue, skb);
+	spin_unlock_bh(&txqi->queue.lock);
+
 	drv_wake_tx_queue(local, txqi);
 
 	return;

@@ -1294,6 +1300,9 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
 	if (!skb)
 		goto out;
 
+	txq->qdepth--;
+	txq->qsize -= skb->len;
+
 	atomic_dec(&sdata->txqs_len[ac]);
 	if (__netif_subqueue_stopped(sdata->dev, ac))
 		ieee80211_propagate_queue_wake(local, sdata->vif.hw_queue[ac]);

-- 
2.1.4

On 2016-01-21 14:23, Michal Kazior wrote:

This will allow drivers to make more educated
decisions whether to defer transmission or not.

Relying on wake_tx_queue() call count implicitly
was not possible because it could be called
without queued frame count actually changing on
software tx aggregation start/stop code paths.

It was also not possible to know how long
byte-wise queue was without dequeueing.

Signed-off-by: Michal Kazior <redacted>

Instead of exposing these in the struct to the driver directly, please
make a function to get them. Since the number of frames is already
tracked in txqi->queue, you can avoid counter duplication that way.
Also, that way you can fix a race condition between accessing the number
of frames counter and the bytes counter.

- Felix

On 26 January 2016 at 11:45, Felix Fietkau [off-list ref] wrote:

On 2016-01-21 14:23, Michal Kazior wrote:

quoted

This will allow drivers to make more educated
decisions whether to defer transmission or not.

Relying on wake_tx_queue() call count implicitly
was not possible because it could be called
without queued frame count actually changing on
software tx aggregation start/stop code paths.

It was also not possible to know how long
byte-wise queue was without dequeueing.

Signed-off-by: Michal Kazior <redacted>

Instead of exposing these in the struct to the driver directly, please
make a function to get them. Since the number of frames is already
tracked in txqi->queue, you can avoid counter duplication that way.

Hmm, so you suggest to have something like:

 void
 ieee80211_get_txq_depth(struct ieee80211_txq *txq,
                         unsigned int *frame_cnt,
                         unsigned int *byte_count) {
   struct txq_info *txqi = txq_to_info(txq);

   if (frame_cnt)
     *frame_cnt = txqi->queue.qlen;

   if (byte_count)
     *byte_cnt = txqi->byte_cnt;
 }

Correct?

Sounds reasonable.

Also, that way you can fix a race condition between accessing the number
of frames counter and the bytes counter.

I don't see a point in maintaining coherency between the two counters
with regard to each other alone. Do you have a use-case that would
actually make use of that property?

I'd like to avoid any unnecessary spinlocks.


Michał

On 2016-01-26 12:56, Michal Kazior wrote:

On 26 January 2016 at 11:45, Felix Fietkau [off-list ref] wrote:

quoted

On 2016-01-21 14:23, Michal Kazior wrote:

quoted

This will allow drivers to make more educated
decisions whether to defer transmission or not.

Relying on wake_tx_queue() call count implicitly
was not possible because it could be called
without queued frame count actually changing on
software tx aggregation start/stop code paths.

It was also not possible to know how long
byte-wise queue was without dequeueing.

Signed-off-by: Michal Kazior <redacted>

Instead of exposing these in the struct to the driver directly, please
make a function to get them. Since the number of frames is already
tracked in txqi->queue, you can avoid counter duplication that way.

Hmm, so you suggest to have something like:

 void
 ieee80211_get_txq_depth(struct ieee80211_txq *txq,
                         unsigned int *frame_cnt,
                         unsigned int *byte_count) {
   struct txq_info *txqi = txq_to_info(txq);

   if (frame_cnt)
     *frame_cnt = txqi->queue.qlen;

   if (byte_count)
     *byte_cnt = txqi->byte_cnt;
 }

Correct?

Sounds reasonable.

Right.

quoted

Also, that way you can fix a race condition between accessing the number
of frames counter and the bytes counter.

I don't see a point in maintaining coherency between the two counters
with regard to each other alone. Do you have a use-case that would
actually make use of that property?

I'd like to avoid any unnecessary spinlocks.

OK. I guess we can leave them out for now. How frequently are you going
to call this function?

- Felix

On 26 January 2016 at 13:04, Felix Fietkau [off-list ref] wrote:

On 2016-01-26 12:56, Michal Kazior wrote:

quoted

On 26 January 2016 at 11:45, Felix Fietkau [off-list ref] wrote:

quoted

On 2016-01-21 14:23, Michal Kazior wrote:

quoted

This will allow drivers to make more educated
decisions whether to defer transmission or not.

Relying on wake_tx_queue() call count implicitly
was not possible because it could be called
without queued frame count actually changing on
software tx aggregation start/stop code paths.

It was also not possible to know how long
byte-wise queue was without dequeueing.

Signed-off-by: Michal Kazior <redacted>

Instead of exposing these in the struct to the driver directly, please
make a function to get them. Since the number of frames is already
tracked in txqi->queue, you can avoid counter duplication that way.

Hmm, so you suggest to have something like:

[...]

quoted

quoted

Also, that way you can fix a race condition between accessing the number
of frames counter and the bytes counter.

I don't see a point in maintaining coherency between the two counters
with regard to each other alone. Do you have a use-case that would
actually make use of that property?

I'd like to avoid any unnecessary spinlocks.

OK. I guess we can leave them out for now. How frequently are you going
to call this function?

Depends, but with new data path in ath10k[1][2] it'll be at least once
for each wake_tx_queue() + once for each txq in each fetch-ind event.


Michał

[1]: https://patchwork.kernel.org/patch/8081301/
[2]: https://patchwork.kernel.org/patch/8081331/

I don't see a point in maintaining coherency between the two counters
with regard to each other alone. Do you have a use-case that would
actually make use of that property?

I'd like to avoid any unnecessary spinlocks.

Make sure you document the lack of consistency between the two values
then, please.

johannes

On 01/21/2016 05:23 AM, Michal Kazior wrote:

The driver can access the queue simultanously
while mac80211 tears down the interface. Without
spinlock protection this could lead to corrupting
sk_buff_head and subsequently to an invalid
pointer dereference.

Hard to know for certain, but this *appears* to fix the unexpectedly large
amount of CE/AXI ath10k firmware crashes that we saw in the 4.2 kernel (4.0 previously
ran much better han 4.2 for us).

We'll continue testing, in case we are just getting lucky so far.

Thanks,
Ben

-- 
Ben Greear [off-list ref]
Candela Technologies Inc  http://www.candelatech.com

On 25 January 2016 at 18:59, Ben Greear [off-list ref] wrote:

On 01/21/2016 05:23 AM, Michal Kazior wrote:

quoted

The driver can access the queue simultanously
while mac80211 tears down the interface. Without
spinlock protection this could lead to corrupting
sk_buff_head and subsequently to an invalid
pointer dereference.

Hard to know for certain, but this *appears* to fix the unexpectedly large
amount of CE/AXI ath10k firmware crashes that we saw in the 4.2 kernel (4.0
previously
ran much better han 4.2 for us).

That's impossible.

Without wake_tx_queue() txqs aren't even allocated (sdata->vif.txq is NULL).


Michał

On 01/25/2016 10:35 PM, Michal Kazior wrote:

On 25 January 2016 at 18:59, Ben Greear [off-list ref] wrote:

quoted

On 01/21/2016 05:23 AM, Michal Kazior wrote:

quoted

The driver can access the queue simultanously
while mac80211 tears down the interface. Without
spinlock protection this could lead to corrupting
sk_buff_head and subsequently to an invalid
pointer dereference.

Hard to know for certain, but this *appears* to fix the unexpectedly large
amount of CE/AXI ath10k firmware crashes that we saw in the 4.2 kernel (4.0
previously
ran much better han 4.2 for us).

That's impossible.

Without wake_tx_queue() txqs aren't even allocated (sdata->vif.txq is NULL).

You are right.  But while testing, one of my guys did find a way to reproduce the
crash very quickly in 4.2.  Happens fastest when I use the HTT-MGT variant
of my firmware, but same firmware works good-ish in 4.0.  Seems I have something
to bisect now if I can get a minimal patch to apply each time to enable my
htt-mgt firmware feature...

The latest test case is to just to change the channel of the AP while station
is connected.  Station sends some null-funcs, firmware resets it's low-level
stuff a bunch because it doesn't get AKCs, then CE/AXI crashes.  Could be
my firmware or kernel modifications of course, though similar crash scenarios have been seen forever
in all sorts of firmwares and kernels.

Thanks,
Ben


-- 
Ben Greear [off-list ref]
Candela Technologies Inc  http://www.candelatech.com

On Thu, 2016-01-21 at 14:23 +0100, Michal Kazior wrote:

The driver can access the queue simultanously
while mac80211 tears down the interface. Without
spinlock protection this could lead to corrupting
sk_buff_head and subsequently to an invalid
pointer dereference.

Applied.

johannes

This will allow drivers to make more educated
decisions whether to defer transmission or not.

Relying on wake_tx_queue() call count implicitly
was not possible because it could be called
without queued frame count actually changing on
software tx aggregation start/stop code paths.

It was also not possible to know how long
byte-wise queue was without dequeueing.

Signed-off-by: Michal Kazior <redacted>
---

Notes:
    v2:
     * export a dedicated API call to get both
       frame/byte counts to reduce redundancy and
       offer possible synchronized reads in the future [Felix]

 include/net/mac80211.h     | 15 +++++++++++++++
 net/mac80211/ieee80211_i.h |  1 +
 net/mac80211/iface.c       |  1 +
 net/mac80211/sta_info.c    |  1 +
 net/mac80211/tx.c          |  8 +++++++-
 net/mac80211/util.c        | 14 ++++++++++++++
 6 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 31337f81ec03..6617516a276f 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h

@@ -5594,4 +5594,19 @@ void ieee80211_unreserve_tid(struct ieee80211_sta *sta, u8 tid);
  */
 struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
 				     struct ieee80211_txq *txq);
+
+/**
+ * ieee80211_txq_get_depth - get pending frame/byte count of given txq
+ *
+ * The values are not guaranteed to be coherent with regard to each other, i.e.
+ * txq state can change half-way of this function and the caller may end up
+ * with "new" frame_cnt and "old" byte_cnt or vice-versa.
+ *
+ * @txq: pointer obtained from station or virtual interface
+ * @frame_cnt: pointer to store frame count
+ * @byte_cnt: pointer to store byte count
+ */
+void ieee80211_txq_get_depth(struct ieee80211_txq *txq,
+			     unsigned long *frame_cnt,
+			     unsigned long *byte_cnt);
 #endif /* MAC80211_H */

diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index a29f61dc9c06..a96f8c0461f6 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h

@@ -804,6 +804,7 @@ enum txq_info_flags {
 struct txq_info {
 	struct sk_buff_head queue;
 	unsigned long flags;
+	unsigned long byte_cnt;
 
 	/* keep last! */
 	struct ieee80211_txq txq;

diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 0451f120746e..453b4e741780 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c

@@ -979,6 +979,7 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
 
 		spin_lock_bh(&txqi->queue.lock);
 		ieee80211_purge_tx_queue(&local->hw, &txqi->queue);
+		txqi->byte_cnt = 0;
 		spin_unlock_bh(&txqi->queue.lock);
 
 		atomic_set(&sdata->txqs_len[txqi->txq.ac], 0);

diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index 7e007cf12cb2..e1d9ccc5d197 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c

@@ -116,6 +116,7 @@ static void __cleanup_single_sta(struct sta_info *sta)
 
 			ieee80211_purge_tx_queue(&local->hw, &txqi->queue);
 			atomic_sub(n, &sdata->txqs_len[txqi->txq.ac]);
+			txqi->byte_cnt = 0;
 		}
 	}
 

diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 3311ce0f3d6c..af584f7cdd63 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c

@@ -1266,7 +1266,11 @@ static void ieee80211_drv_tx(struct ieee80211_local *local,
 	if (atomic_read(&sdata->txqs_len[ac]) >= local->hw.txq_ac_max_pending)
 		netif_stop_subqueue(sdata->dev, ac);
 
-	skb_queue_tail(&txqi->queue, skb);
+	spin_lock_bh(&txqi->queue.lock);
+	txqi->byte_cnt += skb->len;
+	__skb_queue_tail(&txqi->queue, skb);
+	spin_unlock_bh(&txqi->queue.lock);
+
 	drv_wake_tx_queue(local, txqi);
 
 	return;

@@ -1294,6 +1298,8 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
 	if (!skb)
 		goto out;
 
+	txqi->byte_cnt -= skb->len;
+
 	atomic_dec(&sdata->txqs_len[ac]);
 	if (__netif_subqueue_stopped(sdata->dev, ac))
 		ieee80211_propagate_queue_wake(local, sdata->vif.hw_queue[ac]);

diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index fb90d9c5df59..091f3dd62ad1 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c

@@ -3368,3 +3368,17 @@ void ieee80211_init_tx_queue(struct ieee80211_sub_if_data *sdata,
 		txqi->txq.ac = IEEE80211_AC_BE;
 	}
 }
+
+void ieee80211_txq_get_depth(struct ieee80211_txq *txq,
+			     unsigned long *frame_cnt,
+			     unsigned long *byte_cnt)
+{
+	struct txq_info *txqi = to_txq_info(txq);
+
+	if (frame_cnt)
+		*frame_cnt = txqi->queue.qlen;
+
+	if (byte_cnt)
+		*byte_cnt = txqi->byte_cnt;
+}
+EXPORT_SYMBOL(ieee80211_txq_get_depth);

-- 
2.1.4

On Wed, 2016-01-27 at 15:26 +0100, Michal Kazior wrote:

This *looks* a bit worrying - you have an atomic dec for the # of
packets and a non-atomic one for the bytes.

You probably thought about it and I guess it's fine, but can you
explain it?

johannes

On 27 January 2016 at 15:26, Johannes Berg [off-list ref] wrote:

On Wed, 2016-01-27 at 15:26 +0100, Michal Kazior wrote:

quoted

@@ -1294,6 +1298,8 @@ struct sk_buff *ieee80211_tx_dequeue(struct

ieee80211_hw *hw,
      if (!skb)
              goto out;

+     txqi->byte_cnt -= skb->len;
+
      atomic_dec(&sdata->txqs_len[ac]);

This *looks* a bit worrying - you have an atomic dec for the # of
packets and a non-atomic one for the bytes.

You probably thought about it and I guess it's fine, but can you
explain it?

The atomic was used because it accesses per-vif counters. You can't
use txqi->queue.lock for that.

On the other hand byte_cnt is per txqi and it was very easy to make
use of the txqi->queue.lock (which was already acquired in both drv_tx
and tx_dequeue functions). Using atomic* for byte_cnt would be
wasteful a bit.


Michał

On Wed, 2016-01-27 at 15:33 +0100, Michal Kazior wrote:

On 27 January 2016 at 15:26, Johannes Berg <johannes@sipsolutions.net

quoted

wrote:
On Wed, 2016-01-27 at 15:26 +0100, Michal Kazior wrote:

quoted

@@ -1294,6 +1298,8 @@ struct sk_buff *ieee80211_tx_dequeue(struct

ieee80211_hw *hw,
      if (!skb)
              goto out;

+     txqi->byte_cnt -= skb->len;
+
      atomic_dec(&sdata->txqs_len[ac]);

This *looks* a bit worrying - you have an atomic dec for the # of
packets and a non-atomic one for the bytes.

You probably thought about it and I guess it's fine, but can you
explain it?

The atomic was used because it accesses per-vif counters. You can't
use txqi->queue.lock for that.

Ah. I completely missed that distinction, thanks.

johannes

On Wed, 2016-01-27 at 15:26 +0100, Michal Kazior wrote:

This will allow drivers to make more educated
decisions whether to defer transmission or not.

Relying on wake_tx_queue() call count implicitly
was not possible because it could be called
without queued frame count actually changing on
software tx aggregation start/stop code paths.

It was also not possible to know how long
byte-wise queue was without dequeueing.

Applied.

johannes