[PATCH 0/3] ath10k patches

[PATCH 0/3] ath10k patches

[Sujith Manoharan]

From: Sujith Manoharan <c_manoha@qca.qualcomm.com>

Fixes for WEP.

Sujith Manoharan (3):
  ath10k: Fix shared WEP
  ath10k: Fix locking for WEP keys
  ath10k: Fix bug reported by lockdep

 drivers/net/wireless/ath/ath10k/mac.c | 31 +++++++++++++++++++++++++++++--
 drivers/net/wireless/ath/ath10k/mac.h |  2 ++
 drivers/net/wireless/ath/ath10k/wmi.c | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 64 insertions(+), 2 deletions(-)

-- 
2.1.3

[PATCH 0/3] ath10k patches

[Sujith Manoharan]

From: Sujith Manoharan <c_manoha@qca.qualcomm.com>

Fixes for WEP.

Sujith Manoharan (3):
  ath10k: Fix shared WEP
  ath10k: Fix locking for WEP keys
  ath10k: Fix bug reported by lockdep

 drivers/net/wireless/ath/ath10k/mac.c | 31 +++++++++++++++++++++++++++++--
 drivers/net/wireless/ath/ath10k/mac.h |  2 ++
 drivers/net/wireless/ath/ath10k/wmi.c | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 64 insertions(+), 2 deletions(-)

-- 
2.1.3


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

[PATCH 1/3] ath10k: Fix shared WEP

[Sujith Manoharan]

From: Sujith Manoharan <c_manoha@qca.qualcomm.com>

When static keys are used in shared WEP, when a
station is associated, message 3 is sent with an
encrypted payload. But, for subsequent
authentications that are triggered without a
deauth, the auth frame is decrypted by the HW.

To handle this, check if the WEP keys have already
been set for the peer and if so, mark the
frame as decrypted. This scenario can happen
when a station changes its default TX key and initiates
a new authentication sequence.

Signed-off-by: Sujith Manoharan <c_manoha@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath10k/mac.c | 23 +++++++++++++++++++++++
 drivers/net/wireless/ath/ath10k/mac.h |  2 ++
 drivers/net/wireless/ath/ath10k/wmi.c | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 58 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 1245ac8..23116c2 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -179,6 +179,29 @@ static int ath10k_clear_peer_keys(struct ath10k_vif *arvif,
 	return first_errno;
 }
 
+bool ath10k_mac_is_peer_wep_key_set(struct ath10k *ar, const u8 *addr,
+				    u8 keyidx)
+{
+	struct ath10k_peer *peer;
+	int i;
+
+	/* We don't know which vdev this peer belongs to,
+	 * since WMI doesn't give us that information.
+	 *
+	 * FIXME: multi-bss needs to be handled.
+	 */
+	peer = ath10k_peer_find(ar, 0, addr);
+	if (!peer)
+		return false;
+
+	for (i = 0; i < ARRAY_SIZE(peer->keys); i++) {
+		if (peer->keys[i] && peer->keys[i]->keyidx == keyidx)
+			return true;
+	}
+
+	return false;
+}
+
 static int ath10k_clear_vdev_key(struct ath10k_vif *arvif,
 				 struct ieee80211_key_conf *key)
 {
diff --git a/drivers/net/wireless/ath/ath10k/mac.h b/drivers/net/wireless/ath/ath10k/mac.h
index 4e3c989..cfa4d5d 100644
--- a/drivers/net/wireless/ath/ath10k/mac.h
+++ b/drivers/net/wireless/ath/ath10k/mac.h
@@ -41,6 +41,8 @@ void ath10k_mgmt_over_wmi_tx_work(struct work_struct *work);
 void ath10k_halt(struct ath10k *ar);
 void ath10k_mac_vif_beacon_free(struct ath10k_vif *arvif);
 void ath10k_drain_tx(struct ath10k *ar);
+bool ath10k_mac_is_peer_wep_key_set(struct ath10k *ar, const u8 *addr,
+				    u8 keyidx);
 
 static inline struct ath10k_vif *ath10k_vif_to_arvif(struct ieee80211_vif *vif)
 {
diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index c2bc828..a12bba4 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -1113,6 +1113,37 @@ static inline u8 get_rate_idx(u32 rate, enum ieee80211_band band)
 	return rate_idx;
 }
 
+static void ath10k_wmi_handle_wep_reauth(struct ath10k *ar,
+					 struct sk_buff *skb,
+					 struct ieee80211_rx_status *status)
+{
+	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
+	unsigned int hdrlen;
+	bool peer_key;
+	u8 *addr, keyidx;
+
+	if (!ieee80211_is_auth(hdr->frame_control) ||
+	    !ieee80211_has_protected(hdr->frame_control))
+		return;
+
+	hdrlen = ieee80211_hdrlen(hdr->frame_control);
+	if (skb->len < (hdrlen + IEEE80211_WEP_IV_LEN))
+		return;
+
+	keyidx = skb->data[hdrlen + 3] >> 6;
+	addr = ieee80211_get_SA(hdr);
+
+	spin_lock_bh(&ar->data_lock);
+	peer_key = ath10k_mac_is_peer_wep_key_set(ar, addr, keyidx);
+	spin_unlock_bh(&ar->data_lock);
+
+	if (peer_key) {
+		ath10k_dbg(ar, ATH10K_DBG_MAC,
+			   "wep key present for peer: %pM\n", addr);
+		status->flag |= RX_FLAG_DECRYPTED;
+	}
+}
+
 static int ath10k_wmi_event_mgmt_rx(struct ath10k *ar, struct sk_buff *skb)
 {
 	struct wmi_mgmt_rx_event_v1 *ev_v1;
@@ -1200,6 +1231,8 @@ static int ath10k_wmi_event_mgmt_rx(struct ath10k *ar, struct sk_buff *skb)
 	hdr = (struct ieee80211_hdr *)skb->data;
 	fc = le16_to_cpu(hdr->frame_control);
 
+	ath10k_wmi_handle_wep_reauth(ar, skb, status);
+
 	/* FW delivers WEP Shared Auth frame with Protected Bit set and
 	 * encrypted payload. However in case of PMF it delivers decrypted
 	 * frames with Protected Bit set. */
-- 
2.1.3

[PATCH 1/3] ath10k: Fix shared WEP

[Sujith Manoharan]

From: Sujith Manoharan <c_manoha@qca.qualcomm.com>

When static keys are used in shared WEP, when a
station is associated, message 3 is sent with an
encrypted payload. But, for subsequent
authentications that are triggered without a
deauth, the auth frame is decrypted by the HW.

To handle this, check if the WEP keys have already
been set for the peer and if so, mark the
frame as decrypted. This scenario can happen
when a station changes its default TX key and initiates
a new authentication sequence.

Signed-off-by: Sujith Manoharan <c_manoha@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath10k/mac.c | 23 +++++++++++++++++++++++
 drivers/net/wireless/ath/ath10k/mac.h |  2 ++
 drivers/net/wireless/ath/ath10k/wmi.c | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 58 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 1245ac8..23116c2 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -179,6 +179,29 @@ static int ath10k_clear_peer_keys(struct ath10k_vif *arvif,
 	return first_errno;
 }
 
+bool ath10k_mac_is_peer_wep_key_set(struct ath10k *ar, const u8 *addr,
+				    u8 keyidx)
+{
+	struct ath10k_peer *peer;
+	int i;
+
+	/* We don't know which vdev this peer belongs to,
+	 * since WMI doesn't give us that information.
+	 *
+	 * FIXME: multi-bss needs to be handled.
+	 */
+	peer = ath10k_peer_find(ar, 0, addr);
+	if (!peer)
+		return false;
+
+	for (i = 0; i < ARRAY_SIZE(peer->keys); i++) {
+		if (peer->keys[i] && peer->keys[i]->keyidx == keyidx)
+			return true;
+	}
+
+	return false;
+}
+
 static int ath10k_clear_vdev_key(struct ath10k_vif *arvif,
 				 struct ieee80211_key_conf *key)
 {
diff --git a/drivers/net/wireless/ath/ath10k/mac.h b/drivers/net/wireless/ath/ath10k/mac.h
index 4e3c989..cfa4d5d 100644
--- a/drivers/net/wireless/ath/ath10k/mac.h
+++ b/drivers/net/wireless/ath/ath10k/mac.h
@@ -41,6 +41,8 @@ void ath10k_mgmt_over_wmi_tx_work(struct work_struct *work);
 void ath10k_halt(struct ath10k *ar);
 void ath10k_mac_vif_beacon_free(struct ath10k_vif *arvif);
 void ath10k_drain_tx(struct ath10k *ar);
+bool ath10k_mac_is_peer_wep_key_set(struct ath10k *ar, const u8 *addr,
+				    u8 keyidx);
 
 static inline struct ath10k_vif *ath10k_vif_to_arvif(struct ieee80211_vif *vif)
 {
diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index c2bc828..a12bba4 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -1113,6 +1113,37 @@ static inline u8 get_rate_idx(u32 rate, enum ieee80211_band band)
 	return rate_idx;
 }
 
+static void ath10k_wmi_handle_wep_reauth(struct ath10k *ar,
+					 struct sk_buff *skb,
+					 struct ieee80211_rx_status *status)
+{
+	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
+	unsigned int hdrlen;
+	bool peer_key;
+	u8 *addr, keyidx;
+
+	if (!ieee80211_is_auth(hdr->frame_control) ||
+	    !ieee80211_has_protected(hdr->frame_control))
+		return;
+
+	hdrlen = ieee80211_hdrlen(hdr->frame_control);
+	if (skb->len < (hdrlen + IEEE80211_WEP_IV_LEN))
+		return;
+
+	keyidx = skb->data[hdrlen + 3] >> 6;
+	addr = ieee80211_get_SA(hdr);
+
+	spin_lock_bh(&ar->data_lock);
+	peer_key = ath10k_mac_is_peer_wep_key_set(ar, addr, keyidx);
+	spin_unlock_bh(&ar->data_lock);
+
+	if (peer_key) {
+		ath10k_dbg(ar, ATH10K_DBG_MAC,
+			   "wep key present for peer: %pM\n", addr);
+		status->flag |= RX_FLAG_DECRYPTED;
+	}
+}
+
 static int ath10k_wmi_event_mgmt_rx(struct ath10k *ar, struct sk_buff *skb)
 {
 	struct wmi_mgmt_rx_event_v1 *ev_v1;
@@ -1200,6 +1231,8 @@ static int ath10k_wmi_event_mgmt_rx(struct ath10k *ar, struct sk_buff *skb)
 	hdr = (struct ieee80211_hdr *)skb->data;
 	fc = le16_to_cpu(hdr->frame_control);
 
+	ath10k_wmi_handle_wep_reauth(ar, skb, status);
+
 	/* FW delivers WEP Shared Auth frame with Protected Bit set and
 	 * encrypted payload. However in case of PMF it delivers decrypted
 	 * frames with Protected Bit set. */
-- 
2.1.3


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 1/3] ath10k: Fix shared WEP

[Kalle Valo]

Sujith Manoharan <sujith@msujith.org> writes:

> From: Sujith Manoharan <c_manoha@qca.qualcomm.com>
>
> When static keys are used in shared WEP, when a
> station is associated, message 3 is sent with an
> encrypted payload. But, for subsequent
> authentications that are triggered without a
> deauth, the auth frame is decrypted by the HW.
>
> To handle this, check if the WEP keys have already
> been set for the peer and if so, mark the
> frame as decrypted. This scenario can happen
> when a station changes its default TX key and initiates
> a new authentication sequence.
>
> Signed-off-by: Sujith Manoharan <c_manoha@qca.qualcomm.com>

[...]

> +bool ath10k_mac_is_peer_wep_key_set(struct ath10k *ar, const u8 *addr,
> +				    u8 keyidx)
> +{
> +	struct ath10k_peer *peer;
> +	int i;
> +
> +	/* We don't know which vdev this peer belongs to,
> +	 * since WMI doesn't give us that information.
> +	 *
> +	 * FIXME: multi-bss needs to be handled.
> +	 */
> +	peer = ath10k_peer_find(ar, 0, addr);
> +	if (!peer)
> +		return false;

This requires that data_lock is taken, right? If yes, can you document
that with lockdep_assert_held(), please?

> +static void ath10k_wmi_handle_wep_reauth(struct ath10k *ar,
> +					 struct sk_buff *skb,
> +					 struct ieee80211_rx_status *status)
> +{
> +	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
> +	unsigned int hdrlen;
> +	bool peer_key;
> +	u8 *addr, keyidx;

Some kind of comment to document what this workaround is about would be
good to have. For example, the first paragraph from the commit log
sounds perfect.

> +
> +	if (!ieee80211_is_auth(hdr->frame_control) ||
> +	    !ieee80211_has_protected(hdr->frame_control))
> +		return;
> +
> +	hdrlen = ieee80211_hdrlen(hdr->frame_control);
> +	if (skb->len < (hdrlen + IEEE80211_WEP_IV_LEN))
> +		return;
> +
> +	keyidx = skb->data[hdrlen + 3] >> 6;

No magic numbers, please. I didn't find anything for IV internals from
ieee80211.h, not sure if we should add them there or just use ath10k
internal defines.

> +	addr = ieee80211_get_SA(hdr);
> +
> +	spin_lock_bh(&ar->data_lock);
> +	peer_key = ath10k_mac_is_peer_wep_key_set(ar, addr, keyidx);
> +	spin_unlock_bh(&ar->data_lock);
> +
> +	if (peer_key) {
> +		ath10k_dbg(ar, ATH10K_DBG_MAC,
> +			   "wep key present for peer: %pM\n", addr);

"mac wep key present for peer %pM"

-- 
Kalle Valo

Re: [PATCH 1/3] ath10k: Fix shared WEP

[Kalle Valo]

Sujith Manoharan <sujith@msujith.org> writes:

> From: Sujith Manoharan <c_manoha@qca.qualcomm.com>
>
> When static keys are used in shared WEP, when a
> station is associated, message 3 is sent with an
> encrypted payload. But, for subsequent
> authentications that are triggered without a
> deauth, the auth frame is decrypted by the HW.
>
> To handle this, check if the WEP keys have already
> been set for the peer and if so, mark the
> frame as decrypted. This scenario can happen
> when a station changes its default TX key and initiates
> a new authentication sequence.
>
> Signed-off-by: Sujith Manoharan <c_manoha@qca.qualcomm.com>

[...]

> +bool ath10k_mac_is_peer_wep_key_set(struct ath10k *ar, const u8 *addr,
> +				    u8 keyidx)
> +{
> +	struct ath10k_peer *peer;
> +	int i;
> +
> +	/* We don't know which vdev this peer belongs to,
> +	 * since WMI doesn't give us that information.
> +	 *
> +	 * FIXME: multi-bss needs to be handled.
> +	 */
> +	peer = ath10k_peer_find(ar, 0, addr);
> +	if (!peer)
> +		return false;

This requires that data_lock is taken, right? If yes, can you document
that with lockdep_assert_held(), please?

> +static void ath10k_wmi_handle_wep_reauth(struct ath10k *ar,
> +					 struct sk_buff *skb,
> +					 struct ieee80211_rx_status *status)
> +{
> +	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
> +	unsigned int hdrlen;
> +	bool peer_key;
> +	u8 *addr, keyidx;

Some kind of comment to document what this workaround is about would be
good to have. For example, the first paragraph from the commit log
sounds perfect.

> +
> +	if (!ieee80211_is_auth(hdr->frame_control) ||
> +	    !ieee80211_has_protected(hdr->frame_control))
> +		return;
> +
> +	hdrlen = ieee80211_hdrlen(hdr->frame_control);
> +	if (skb->len < (hdrlen + IEEE80211_WEP_IV_LEN))
> +		return;
> +
> +	keyidx = skb->data[hdrlen + 3] >> 6;

No magic numbers, please. I didn't find anything for IV internals from
ieee80211.h, not sure if we should add them there or just use ath10k
internal defines.

> +	addr = ieee80211_get_SA(hdr);
> +
> +	spin_lock_bh(&ar->data_lock);
> +	peer_key = ath10k_mac_is_peer_wep_key_set(ar, addr, keyidx);
> +	spin_unlock_bh(&ar->data_lock);
> +
> +	if (peer_key) {
> +		ath10k_dbg(ar, ATH10K_DBG_MAC,
> +			   "wep key present for peer: %pM\n", addr);

"mac wep key present for peer %pM"

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 1/3] ath10k: Fix shared WEP

[Sujith Manoharan]

Kalle Valo wrote:
> No magic numbers, please. I didn't find anything for IV internals from
> ieee80211.h, not sure if we should add them there or just use ath10k
> internal defines.

We are just retrieving the keyidx from the IV - not sure adding a couple
of macros for "6" and "3" will make any difference.

Sujith

Re: [PATCH 1/3] ath10k: Fix shared WEP

[Sujith Manoharan]

Kalle Valo wrote:
> No magic numbers, please. I didn't find anything for IV internals from
> ieee80211.h, not sure if we should add them there or just use ath10k
> internal defines.

We are just retrieving the keyidx from the IV - not sure adding a couple
of macros for "6" and "3" will make any difference.

Sujith

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 1/3] ath10k: Fix shared WEP

[Kalle Valo]

Sujith Manoharan <sujith@msujith.org> writes:

> Kalle Valo wrote:
>> No magic numbers, please. I didn't find anything for IV internals from
>> ieee80211.h, not sure if we should add them there or just use ath10k
>> internal defines.
>
> We are just retrieving the keyidx from the IV - not sure adding a couple
> of macros for "6" and "3" will make any difference.

It won't make a difference for you but it will for someone else reading
that part of code.

keyidx = skb->data[hdrlen + WEP_HDR_IV_LEN] >> WEP_HDR_KEY_ID_LSB;

versus:

keyidx = skb->data[hdrlen + 3] >> 6;

(I didn't check the standard so tha names can be totally wrong.)

-- 
Kalle Valo

Re: [PATCH 1/3] ath10k: Fix shared WEP

[Kalle Valo]

Sujith Manoharan <sujith@msujith.org> writes:

> Kalle Valo wrote:
>> No magic numbers, please. I didn't find anything for IV internals from
>> ieee80211.h, not sure if we should add them there or just use ath10k
>> internal defines.
>
> We are just retrieving the keyidx from the IV - not sure adding a couple
> of macros for "6" and "3" will make any difference.

It won't make a difference for you but it will for someone else reading
that part of code.

keyidx = skb->data[hdrlen + WEP_HDR_IV_LEN] >> WEP_HDR_KEY_ID_LSB;

versus:

keyidx = skb->data[hdrlen + 3] >> 6;

(I didn't check the standard so tha names can be totally wrong.)

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

[PATCH 2/3] ath10k: Fix locking for WEP keys

[Sujith Manoharan]

From: Sujith Manoharan <c_manoha@qca.qualcomm.com>

peer->keys needs to be protected by data_lock
since it is also accessed from the WMI path.

Both install() and clear() routines for peer
keys modify the key contents, so use the data_lock
to avoid races.

Signed-off-by: Sujith Manoharan <c_manoha@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath10k/mac.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 23116c2..2200c64 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -136,7 +136,9 @@ static int ath10k_install_peer_wep_keys(struct ath10k_vif *arvif,
 		if (ret)
 			return ret;
 
+		spin_lock_bh(&ar->data_lock);
 		peer->keys[i] = arvif->wep_keys[i];
+		spin_unlock_bh(&ar->data_lock);
 	}
 
 	return 0;
@@ -173,7 +175,9 @@ static int ath10k_clear_peer_keys(struct ath10k_vif *arvif,
 			ath10k_warn(ar, "failed to remove peer wep key %d: %d\n",
 				    i, ret);
 
+		spin_lock_bh(&ar->data_lock);
 		peer->keys[i] = NULL;
+		spin_unlock_bh(&ar->data_lock);
 	}
 
 	return first_errno;
-- 
2.1.3

[PATCH 2/3] ath10k: Fix locking for WEP keys

[Sujith Manoharan]

From: Sujith Manoharan <c_manoha@qca.qualcomm.com>

peer->keys needs to be protected by data_lock
since it is also accessed from the WMI path.

Both install() and clear() routines for peer
keys modify the key contents, so use the data_lock
to avoid races.

Signed-off-by: Sujith Manoharan <c_manoha@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath10k/mac.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 23116c2..2200c64 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -136,7 +136,9 @@ static int ath10k_install_peer_wep_keys(struct ath10k_vif *arvif,
 		if (ret)
 			return ret;
 
+		spin_lock_bh(&ar->data_lock);
 		peer->keys[i] = arvif->wep_keys[i];
+		spin_unlock_bh(&ar->data_lock);
 	}
 
 	return 0;
@@ -173,7 +175,9 @@ static int ath10k_clear_peer_keys(struct ath10k_vif *arvif,
 			ath10k_warn(ar, "failed to remove peer wep key %d: %d\n",
 				    i, ret);
 
+		spin_lock_bh(&ar->data_lock);
 		peer->keys[i] = NULL;
+		spin_unlock_bh(&ar->data_lock);
 	}
 
 	return first_errno;
-- 
2.1.3


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 2/3] ath10k: Fix locking for WEP keys

[Kalle Valo]

Sujith Manoharan <sujith@msujith.org> writes:

> From: Sujith Manoharan <c_manoha@qca.qualcomm.com>
>
> peer->keys needs to be protected by data_lock
> since it is also accessed from the WMI path.
>
> Both install() and clear() routines for peer
> keys modify the key contents, so use the data_lock
> to avoid races.
>
> Signed-off-by: Sujith Manoharan <c_manoha@qca.qualcomm.com>

Can you also document the field with "protected by data_lock" in core.h?

-- 
Kalle Valo

Re: [PATCH 2/3] ath10k: Fix locking for WEP keys

[Kalle Valo]

Sujith Manoharan <sujith@msujith.org> writes:

> From: Sujith Manoharan <c_manoha@qca.qualcomm.com>
>
> peer->keys needs to be protected by data_lock
> since it is also accessed from the WMI path.
>
> Both install() and clear() routines for peer
> keys modify the key contents, so use the data_lock
> to avoid races.
>
> Signed-off-by: Sujith Manoharan <c_manoha@qca.qualcomm.com>

Can you also document the field with "protected by data_lock" in core.h?

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

[PATCH 3/3] ath10k: Fix bug reported by lockdep

[Sujith Manoharan]

From: Sujith Manoharan <c_manoha@qca.qualcomm.com>

ath10k_tx_wep_key_work() acquires conf_mutex, so
cancelling it when conf_mutex is already taken
in ath10k_remove_interface() is incorrect, so
move it outside the lock.

Snippet from the lockdep report:

kernel: ======================================================
kernel: [ INFO: possible circular locking dependency detected ]
kernel: 3.18.0-rc5-wl-debug #34 Tainted: G           O
kernel: -------------------------------------------------------
kernel: hostapd/451 is trying to acquire lock:
kernel:  ((&arvif->wep_key_work)){+.+...}, at: [<ffffffff810872d5>] flush_work+0x5/0x290
kernel: but task is already holding lock:
kernel:  (&ar->conf_mutex){+.+.+.}, at: [<ffffffffa0b99f00>] ath10k_remove_interface+0x40/0x290 [ath10k_core]
kernel: which lock already depends on the new lock.

Signed-off-by: Sujith Manoharan <c_manoha@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath10k/mac.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 2200c64..651e318 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -3041,10 +3041,10 @@ static void ath10k_remove_interface(struct ieee80211_hw *hw,
 	struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
 	int ret;
 
-	mutex_lock(&ar->conf_mutex);
-
 	cancel_work_sync(&arvif->wep_key_work);
 
+	mutex_lock(&ar->conf_mutex);
+
 	spin_lock_bh(&ar->data_lock);
 	ath10k_mac_vif_beacon_cleanup(arvif);
 	spin_unlock_bh(&ar->data_lock);
-- 
2.1.3

[PATCH 3/3] ath10k: Fix bug reported by lockdep

[Sujith Manoharan]

From: Sujith Manoharan <c_manoha@qca.qualcomm.com>

ath10k_tx_wep_key_work() acquires conf_mutex, so
cancelling it when conf_mutex is already taken
in ath10k_remove_interface() is incorrect, so
move it outside the lock.

Snippet from the lockdep report:

kernel: ======================================================
kernel: [ INFO: possible circular locking dependency detected ]
kernel: 3.18.0-rc5-wl-debug #34 Tainted: G           O
kernel: -------------------------------------------------------
kernel: hostapd/451 is trying to acquire lock:
kernel:  ((&arvif->wep_key_work)){+.+...}, at: [<ffffffff810872d5>] flush_work+0x5/0x290
kernel: but task is already holding lock:
kernel:  (&ar->conf_mutex){+.+.+.}, at: [<ffffffffa0b99f00>] ath10k_remove_interface+0x40/0x290 [ath10k_core]
kernel: which lock already depends on the new lock.

Signed-off-by: Sujith Manoharan <c_manoha@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath10k/mac.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 2200c64..651e318 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -3041,10 +3041,10 @@ static void ath10k_remove_interface(struct ieee80211_hw *hw,
 	struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
 	int ret;
 
-	mutex_lock(&ar->conf_mutex);
-
 	cancel_work_sync(&arvif->wep_key_work);
 
+	mutex_lock(&ar->conf_mutex);
+
 	spin_lock_bh(&ar->data_lock);
 	ath10k_mac_vif_beacon_cleanup(arvif);
 	spin_unlock_bh(&ar->data_lock);
-- 
2.1.3


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k