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 <[email protected]>
---
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
On 26 January 2016 at 11:45, Felix Fietkau <[email protected]> wrote:
> 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 <[email protected]>
> 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 Wed, 2016-01-27 at 15:26 +0100, Michal Kazior wrote:
>
> @@ -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?
johannes
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
On 2016-01-26 12:56, Michal Kazior wrote:
> On 26 January 2016 at 11:45, Felix Fietkau <[email protected]> wrote:
>> 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 <[email protected]>
>> 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.
>> 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
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 <[email protected]>
---
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
> 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 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 <[email protected]>
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 13:04, Felix Fietkau <[email protected]> wrote:
> On 2016-01-26 12:56, Michal Kazior wrote:
>> On 26 January 2016 at 11:45, Felix Fietkau <[email protected]> wrote:
>>> 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 <[email protected]>
>>> 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:
[...]
>>> 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/
On 27 January 2016 at 15:26, Johannes Berg <[email protected]> wrote:
> On Wed, 2016-01-27 at 15:26 +0100, Michal Kazior wrote:
>>
>> @@ -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 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
>
> Fixes: ba8c3d6f16a1 ("mac80211: add an intermediate software queue implementation")
> Signed-off-by: Michal Kazior <[email protected]>
> ---
> 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);
> }
>
>
--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com
On Wed, 2016-01-27 at 15:33 +0100, Michal Kazior wrote:
> On 27 January 2016 at 15:26, Johannes Berg <[email protected]
> > wrote:
> > On Wed, 2016-01-27 at 15:26 +0100, Michal Kazior wrote:
> > >
> > > @@ -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 01/25/2016 10:35 PM, Michal Kazior wrote:
> On 25 January 2016 at 18:59, Ben Greear <[email protected]> wrote:
>> 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).
>
> 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 <[email protected]>
Candela Technologies Inc http://www.candelatech.com
On 25 January 2016 at 18:59, Ben Greear <[email protected]> wrote:
> 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).
That's impossible.
Without wake_tx_queue() txqs aren't even allocated (sdata->vif.txq is NULL).
Michał
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 <[email protected]>
---
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 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