[ath9k-devel] [RFC 0/2] ath10k: fix qos workaround

[ath9k-devel] [RFC 0/2] ath10k: fix qos workaround

[Michal Kazior]

[ath9k-devel] [RFC 1/2] ath10k: make more space in ath10k_skb_cb

[Michal Kazior]

Some fields are used only for the original payload
skbuff, and other are used for htt tx descriptor
skbuff. If we split them and make a union we get a
lot of extra room.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/core.h |   31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index 42d9e72..d5ad574 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -52,24 +52,31 @@ struct ath10k_skb_cb {
 	bool is_mapped;
 	bool is_aborted;
 
-	struct {
-		u8 vdev_id;
-		u16 msdu_id;
-		u8 tid;
-		bool is_offchan;
-		bool is_conf;
-		bool discard;
-		bool no_ack;
-		u8 refcount;
-		struct sk_buff *txfrag;
-		struct sk_buff *msdu;
+	union {
+		struct {
+			u8 vdev_id;
+			u8 tid;
+			bool is_offchan;
+
+			/* 19 bytes left */
+		} __packed;
+
+		struct {
+			u16 msdu_id;
+			bool is_conf;
+			bool discard;
+			bool no_ack;
+			u8 refcount;
+			struct sk_buff *txfrag;
+			struct sk_buff *msdu;
+		} __packed;
 	} __packed htt;
 
 	struct {
 		u8 credits_used;
 	} __packed htc;
 
-	/* 4 bytes left on 64bit arch */
+	/* 7 bytes left on 64bit arch */
 } __packed;
 
 static inline struct ath10k_skb_cb *ATH10K_SKB_CB(struct sk_buff *skb)
-- 
1.7.9.5

[ath9k-devel] [RFC 2/2] ath10k: copy skb during tx

[Michal Kazior]

HW is unable to overwrite Qos Control field from
Qos Data Frames. We must strip the QoS Control
provided by mac80211 before we can submit a frame
to the FW.

Since we modified a frame and never restored it
this could cause some issues. It definietely ended
up showing corrupted frames from a monitor
interface.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/core.h |    1 +
 drivers/net/wireless/ath/ath10k/mac.c  |   21 +++++++++++++++++++--
 drivers/net/wireless/ath/ath10k/txrx.c |    8 +++++---
 3 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index d5ad574..911f367 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -57,6 +57,7 @@ struct ath10k_skb_cb {
 			u8 vdev_id;
 			u8 tid;
 			bool is_offchan;
+			struct sk_buff *orig_skb;
 
 			/* 19 bytes left */
 		} __packed;
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 60855fb..60fe808 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -1275,7 +1275,8 @@ static void ath10k_tx_htt(struct ath10k *ar, struct sk_buff *skb)
 
 	if (ret) {
 		ath10k_warn("tx failed (%d). dropping packet.\n", ret);
-		ieee80211_free_txskb(ar->hw, skb);
+		ieee80211_free_txskb(ar->hw, ATH10K_SKB_CB(skb)->htt.orig_skb);
+		kfree_skb(skb);
 	}
 }
 
@@ -1288,7 +1289,8 @@ void ath10k_offchan_tx_purge(struct ath10k *ar)
 		if (!skb)
 			break;
 
-		ieee80211_free_txskb(ar->hw, skb);
+		ieee80211_free_txskb(ar->hw, ATH10K_SKB_CB(skb)->htt.orig_skb);
+		kfree_skb(skb);
 	}
 }
 
@@ -1472,6 +1474,7 @@ static void ath10k_tx(struct ieee80211_hw *hw,
 	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
 	struct ath10k *ar = hw->priv;
 	struct ath10k_vif *arvif = NULL;
+	struct sk_buff *orig_skb;
 	u32 vdev_id = 0;
 	u8 tid;
 
@@ -1494,6 +1497,19 @@ static void ath10k_tx(struct ieee80211_hw *hw,
 		tid = qc[0] & IEEE80211_QOS_CTL_TID_MASK;
 	}
 
+	/* In most cases (most APs support QoS these days) we need to strip QoS
+	 * Control field (HW quirk). This in turn may confuse mac80211 and
+	 * userspace. Simply saving and restoring QoS Control is not enough
+	 * since after this tx callback is done the frame may be pushed to
+	 * monitor interfaces. The userspace then sees corrupted frames. */
+	orig_skb = skb;
+	skb = skb_copy(orig_skb, GFP_ATOMIC);
+	if (!skb) {
+		ath10k_warn("could not copy skb, dropping\n");
+		ieee80211_free_txskb(hw, orig_skb);
+		return;
+	}
+
 	ath10k_tx_h_qos_workaround(hw, control, skb);
 	ath10k_tx_h_update_wep_key(skb);
 	ath10k_tx_h_add_p2p_noa_ie(ar, skb);
@@ -1502,6 +1518,7 @@ static void ath10k_tx(struct ieee80211_hw *hw,
 	memset(ATH10K_SKB_CB(skb), 0, sizeof(*ATH10K_SKB_CB(skb)));
 	ATH10K_SKB_CB(skb)->htt.vdev_id = vdev_id;
 	ATH10K_SKB_CB(skb)->htt.tid = tid;
+	ATH10K_SKB_CB(skb)->htt.orig_skb = orig_skb;
 
 	if (info->flags & IEEE80211_TX_CTL_TX_OFFCHAN) {
 		spin_lock_bh(&ar->data_lock);
diff --git a/drivers/net/wireless/ath/ath10k/txrx.c b/drivers/net/wireless/ath/ath10k/txrx.c
index 6a66dc2..9c130f4 100644
--- a/drivers/net/wireless/ath/ath10k/txrx.c
+++ b/drivers/net/wireless/ath/ath10k/txrx.c
@@ -50,6 +50,7 @@ void ath10k_txrx_tx_unref(struct htt_struct *htt, struct sk_buff *txdesc)
 	struct ieee80211_tx_info *info;
 	struct sk_buff *txfrag = ATH10K_SKB_CB(txdesc)->htt.txfrag;
 	struct sk_buff *msdu = ATH10K_SKB_CB(txdesc)->htt.msdu;
+	struct sk_buff *orig_skb = ATH10K_SKB_CB(msdu)->htt.orig_skb;
 	int ret;
 
 	if (ATH10K_SKB_CB(txdesc)->htt.refcount == 0)
@@ -76,7 +77,7 @@ void ath10k_txrx_tx_unref(struct htt_struct *htt, struct sk_buff *txdesc)
 	memset(&info->status, 0, sizeof(info->status));
 
 	if (ATH10K_SKB_CB(txdesc)->htt.discard) {
-		ieee80211_free_txskb(htt->ar->hw, msdu);
+		ieee80211_free_txskb(htt->ar->hw, orig_skb);
 		goto exit;
 	}
 
@@ -86,7 +87,7 @@ void ath10k_txrx_tx_unref(struct htt_struct *htt, struct sk_buff *txdesc)
 	if (ATH10K_SKB_CB(txdesc)->htt.no_ack)
 		info->flags &= ~IEEE80211_TX_STAT_ACK;
 
-	ieee80211_tx_status(htt->ar->hw, msdu);
+	ieee80211_tx_status(htt->ar->hw, orig_skb);
 	/* we do not own the msdu anymore */
 
 exit:
@@ -96,7 +97,8 @@ exit:
 		wake_up(&htt->empty_tx_wq);
 	spin_unlock_bh(&htt->tx_lock);
 
-	dev_kfree_skb_any(txdesc);
+	kfree_skb(txdesc);
+	kfree_skb(msdu);
 }
 
 void ath10k_txrx_tx_completed(struct htt_struct *htt,
-- 
1.7.9.5

[ath9k-devel] [RFC 0/2] ath10k: fix qos workaround

[Sujith Manoharan]

Michal Kazior wrote:
> >From what I've observed so far is frames in
> monitor mode (non promiscuous) are corrupted. They
> have the QoS Control stripped, with LLC header
> finding itself in the QoS Control. Wireshark shows
> such packets as A-MSDU corrupted frames. I tried
> restoring the QoS Control but it didn't fix that.

I don't understand. Why should pure monitor mode care about
the TX path ?

Sujith

[ath9k-devel] [RFC 0/2] ath10k: fix qos workaround

[Michal Kazior]

On 24/04/13 12:53, Sujith Manoharan wrote:
> Michal Kazior wrote:
>> >From what I've observed so far is frames in
>> monitor mode (non promiscuous) are corrupted. They
>> have the QoS Control stripped, with LLC header
>> finding itself in the QoS Control. Wireshark shows
>> such packets as A-MSDU corrupted frames. I tried
>> restoring the QoS Control but it didn't fix that.
>
> I don't understand. Why should pure monitor mode care about
> the TX path ?

This is not concerned with the pure (I hope we don't have a 
misunderstanding here) monitor mode.

You can create monitor vif when associated and you might want to listen 
to the traffic *without* going into the promiscuous mode (no monitor 
vdev is created). It simply passes raw 802.11 frames that pass through 
mac80211 (both tx and rx) on other interfaces (i.e. associated station 
interface).


-- Pozdrawiam / Best regards, Michal Kazior.

[ath9k-devel] [RFC 0/2] ath10k: fix qos workaround

[Kalle Valo]

Hi Michal,

Michal Kazior <michal.kazior@tieto.com> writes:

> On 24/04/13 12:53, Sujith Manoharan wrote:
>> Michal Kazior wrote:
>>> >From what I've observed so far is frames in
>>> monitor mode (non promiscuous) are corrupted. They
>>> have the QoS Control stripped, with LLC header
>>> finding itself in the QoS Control. Wireshark shows
>>> such packets as A-MSDU corrupted frames. I tried
>>> restoring the QoS Control but it didn't fix that.
>>
>> I don't understand. Why should pure monitor mode care about
>> the TX path ?
>
> This is not concerned with the pure (I hope we don't have a 
> misunderstanding here) monitor mode.
>
> You can create monitor vif when associated and you might want to listen 
> to the traffic *without* going into the promiscuous mode (no monitor 
> vdev is created). It simply passes raw 802.11 frames that pass through 
> mac80211 (both tx and rx) on other interfaces (i.e. associated station 
> interface).

[ath9k-devel] [RFC 0/2] ath10k: fix qos workaround

[Michal Kazior]

On 25/04/13 08:43, Kalle Valo wrote:
> Hi Michal,
>
> Michal Kazior <michal.kazior@tieto.com> writes:
>
>> On 24/04/13 12:53, Sujith Manoharan wrote:
>>> Michal Kazior wrote:
>>>> >From what I've observed so far is frames in
>>>> monitor mode (non promiscuous) are corrupted. They
>>>> have the QoS Control stripped, with LLC header
>>>> finding itself in the QoS Control. Wireshark shows
>>>> such packets as A-MSDU corrupted frames. I tried
>>>> restoring the QoS Control but it didn't fix that.
>>>
>>> I don't understand. Why should pure monitor mode care about
>>> the TX path ?
>>
>> This is not concerned with the pure (I hope we don't have a
>> misunderstanding here) monitor mode.
>>
>> You can create monitor vif when associated and you might want to listen
>> to the traffic *without* going into the promiscuous mode (no monitor
>> vdev is created). It simply passes raw 802.11 frames that pass through
>> mac80211 (both tx and rx) on other interfaces (i.e. associated station
>> interface).
>
>  From prioritising point of view I consider that mode as a very low
> priority feature. For me most important is to support the real monitor
> mode (iw wlan0 set type monitor), anything else is just nice to have.
>
> I haven't even looked at your patches, but I'm worried that if a very
> low priority feature jeopardizes normal functionality. What if we just
> don't support the monitor mode while associated feature? Can we do that?

The issue would be seen even if we had no explicit monitor mode support 
in the driver. The following:

; ifconfig wlan0 up
; iw wlan0 connect -w dd-wrt-open
; iw wlan0 interface add mon type monitor
; ifconfig mon up
; wireshark -pkni mon

Starts wireshark in non-promiscuous mode. This allows us to see tx/rx 
from wlan0 (and possibly other running interfaces) in raw  instead 
ethernet frames. The driver doesn't even know if `mon` exists, nor if 
the `mon` interface is `up` nor if wireshark is running. mac80211 simply 
passes all traffic as-is to mon interface.

Perhaps this is a 'nice to have' thing, but it also fixes the QoS 
Control stripping issue in one go. We could simply do QoS restoring upon 
tx completion to make mac80211 happy on ieee80211_tx_status(), but the 
aforementioned monitor case will still be broken.

However the patch may introduce tx performance regression due to 
skb_copy(). We could possibly optimize it (allocate skbuff and do memcpy 
instead of memmove for the qos workaround).


-- Pozdrawiam / Best regards, Michal Kazior.