[RFC/RFT 1/2] ath10k: fix monitor start/stop sequences

[RFC/RFT 1/2] ath10k: fix monitor start/stop sequences

[Michal Kazior]

Fix some cases where monitor start failure left
the driver in a confused state.

This also makes the monitor code simpler.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/mac.c | 117 +++++++++++++---------------------
 1 file changed, 45 insertions(+), 72 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index b8314a5..2a1c710 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -492,19 +492,6 @@ static inline int ath10k_vdev_setup_sync(struct ath10k *ar)
 	return 0;
 }
 
-static bool ath10k_monitor_is_enabled(struct ath10k *ar)
-{
-	lockdep_assert_held(&ar->conf_mutex);
-
-	ath10k_dbg(ATH10K_DBG_MAC,
-		   "mac monitor refs: promisc %d monitor %d cac %d\n",
-		   ar->promisc, ar->monitor,
-		   test_bit(ATH10K_CAC_RUNNING, &ar->dev_flags));
-
-	return ar->promisc || ar->monitor ||
-	       test_bit(ATH10K_CAC_RUNNING, &ar->dev_flags);
-}
-
 static int ath10k_monitor_vdev_start(struct ath10k *ar, int vdev_id)
 {
 	struct cfg80211_chan_def *chandef = &ar->chandef;
@@ -654,16 +641,6 @@ static int ath10k_monitor_start(struct ath10k *ar)
 
 	lockdep_assert_held(&ar->conf_mutex);
 
-	if (!ath10k_monitor_is_enabled(ar)) {
-		ath10k_warn("trying to start monitor with no references\n");
-		return 0;
-	}
-
-	if (ar->monitor_started) {
-		ath10k_dbg(ATH10K_DBG_MAC, "mac monitor already started\n");
-		return 0;
-	}
-
 	ret = ath10k_monitor_vdev_create(ar);
 	if (ret) {
 		ath10k_warn("failed to create monitor vdev: %d\n", ret);
@@ -683,34 +660,48 @@ static int ath10k_monitor_start(struct ath10k *ar)
 	return 0;
 }
 
-static void ath10k_monitor_stop(struct ath10k *ar)
+static int ath10k_monitor_stop(struct ath10k *ar)
 {
 	int ret;
 
 	lockdep_assert_held(&ar->conf_mutex);
 
-	if (ath10k_monitor_is_enabled(ar)) {
-		ath10k_dbg(ATH10K_DBG_MAC,
-			   "mac monitor will be stopped later\n");
-		return;
-	}
-
-	if (!ar->monitor_started) {
-		ath10k_dbg(ATH10K_DBG_MAC,
-			   "mac monitor probably failed to start earlier\n");
-		return;
-	}
-
 	ret = ath10k_monitor_vdev_stop(ar);
-	if (ret)
+	if (ret) {
 		ath10k_warn("failed to stop monitor vdev: %d\n", ret);
+		return ret;
+	}
 
 	ret = ath10k_monitor_vdev_delete(ar);
-	if (ret)
+	if (ret) {
 		ath10k_warn("failed to delete monitor vdev: %d\n", ret);
+		return ret;
+	}
 
 	ar->monitor_started = false;
 	ath10k_dbg(ATH10K_DBG_MAC, "mac monitor stopped\n");
+
+	return 0;
+}
+
+static int ath10k_monitor_recalc(struct ath10k *ar)
+{
+	bool should_start;
+
+	should_start = ar->promisc || ar->monitor ||
+		       test_bit(ATH10K_CAC_RUNNING, &ar->dev_flags);
+
+	ath10k_dbg(ATH10K_DBG_MAC,
+		   "mac monitor recalc started? %d should? %d\n",
+		   ar->monitor_started, should_start);
+
+	if (should_start == ar->monitor_started)
+		return 0;
+
+	if (should_start)
+		return ath10k_monitor_start(ar);
+	else
+		return ath10k_monitor_stop(ar);
 }
 
 static int ath10k_recalc_rtscts_prot(struct ath10k_vif *arvif)
@@ -741,7 +732,7 @@ static int ath10k_start_cac(struct ath10k *ar)
 
 	set_bit(ATH10K_CAC_RUNNING, &ar->dev_flags);
 
-	ret = ath10k_monitor_start(ar);
+	ret = ath10k_monitor_recalc(ar);
 	if (ret) {
 		ath10k_warn("failed to start monitor (cac): %d\n", ret);
 		clear_bit(ATH10K_CAC_RUNNING, &ar->dev_flags);
@@ -2316,12 +2307,12 @@ void ath10k_halt(struct ath10k *ar)
 
 	lockdep_assert_held(&ar->conf_mutex);
 
-	if (ath10k_monitor_is_enabled(ar)) {
-		clear_bit(ATH10K_CAC_RUNNING, &ar->dev_flags);
-		ar->promisc = false;
-		ar->monitor = false;
+	clear_bit(ATH10K_CAC_RUNNING, &ar->dev_flags);
+	ar->promisc = false;
+	ar->monitor = false;
+
+	if (ar->monitor_started)
 		ath10k_monitor_stop(ar);
-	}
 
 	del_timer_sync(&ar->scan.timeout);
 	ath10k_reset_scan((unsigned long)ar);
@@ -2574,7 +2565,7 @@ static void ath10k_config_chan(struct ath10k *ar)
 	/* First stop monitor interface. Some FW versions crash if there's a
 	 * lone monitor interface. */
 	if (ar->monitor_started)
-		ath10k_monitor_vdev_stop(ar);
+		ath10k_monitor_stop(ar);
 
 	list_for_each_entry(arvif, &ar->arvifs, list) {
 		if (!arvif->is_started)
@@ -2619,8 +2610,7 @@ static void ath10k_config_chan(struct ath10k *ar)
 		}
 	}
 
-	if (ath10k_monitor_is_enabled(ar))
-		ath10k_monitor_vdev_start(ar, ar->monitor_vdev_id);
+	ath10k_monitor_recalc(ar);
 }
 
 static int ath10k_config(struct ieee80211_hw *hw, u32 changed)
@@ -2675,19 +2665,10 @@ static int ath10k_config(struct ieee80211_hw *hw, u32 changed)
 		ath10k_config_ps(ar);
 
 	if (changed & IEEE80211_CONF_CHANGE_MONITOR) {
-		if (conf->flags & IEEE80211_CONF_MONITOR && !ar->monitor) {
-			ar->monitor = true;
-			ret = ath10k_monitor_start(ar);
-			if (ret) {
-				ath10k_warn("failed to start monitor (config): %d\n",
-					    ret);
-				ar->monitor = false;
-			}
-		} else if (!(conf->flags & IEEE80211_CONF_MONITOR) &&
-			   ar->monitor) {
-			ar->monitor = false;
-			ath10k_monitor_stop(ar);
-		}
+		ar->monitor = conf->flags & IEEE80211_CONF_MONITOR;
+		ret = ath10k_monitor_recalc(ar);
+		if (ret)
+			ath10k_warn("failed to recalc monitor: %d\n", ret);
 	}
 
 	mutex_unlock(&ar->conf_mutex);
@@ -2944,18 +2925,10 @@ static void ath10k_configure_filter(struct ieee80211_hw *hw,
 	*total_flags &= SUPPORTED_FILTERS;
 	ar->filter_flags = *total_flags;
 
-	if (ar->filter_flags & FIF_PROMISC_IN_BSS && !ar->promisc) {
-		ar->promisc = true;
-		ret = ath10k_monitor_start(ar);
-		if (ret) {
-			ath10k_warn("failed to start monitor (promisc): %d\n",
-				    ret);
-			ar->promisc = false;
-		}
-	} else if (!(ar->filter_flags & FIF_PROMISC_IN_BSS) && ar->promisc) {
-		ar->promisc = false;
-		ath10k_monitor_stop(ar);
-	}
+	ar->promisc = !!(ar->filter_flags & FIF_PROMISC_IN_BSS);
+	ret = ath10k_monitor_recalc(ar);
+	if (ret)
+		ath10k_warn("failed to recalc montior: %d\n", ret);
 
 	mutex_unlock(&ar->conf_mutex);
 }
-- 
1.8.5.3

[RFC/RFT 2/2] ath10k: don't start monitor vdev for promisc

[Michal Kazior]

ath10k doesn't apply any extra rx filters so
there's no need to start monitor vdev for
promiscuous mode.

This fixes crashes with 4addr station interface
bridging and some very rare crashes of AP
interfaces with bridging as well.

Reported-by: Sven Schnelle <svens@stackframe.org>
Reported-by: Vu Hai NGUYEN <vh.nguyen@actiasodielec.fr>
Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/core.h | 1 -
 drivers/net/wireless/ath/ath10k/mac.c  | 9 +--------
 2 files changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index 83a5fa9..d5cba97 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -432,7 +432,6 @@ struct ath10k {
 	struct cfg80211_chan_def chandef;
 
 	int free_vdev_map;
-	bool promisc;
 	bool monitor;
 	int monitor_vdev_id;
 	bool monitor_started;
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 2a1c710..f9ab878 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -688,7 +688,7 @@ static int ath10k_monitor_recalc(struct ath10k *ar)
 {
 	bool should_start;
 
-	should_start = ar->promisc || ar->monitor ||
+	should_start = ar->monitor ||
 		       test_bit(ATH10K_CAC_RUNNING, &ar->dev_flags);
 
 	ath10k_dbg(ATH10K_DBG_MAC,
@@ -2308,7 +2308,6 @@ void ath10k_halt(struct ath10k *ar)
 	lockdep_assert_held(&ar->conf_mutex);
 
 	clear_bit(ATH10K_CAC_RUNNING, &ar->dev_flags);
-	ar->promisc = false;
 	ar->monitor = false;
 
 	if (ar->monitor_started)
@@ -2917,7 +2916,6 @@ static void ath10k_configure_filter(struct ieee80211_hw *hw,
 				    u64 multicast)
 {
 	struct ath10k *ar = hw->priv;
-	int ret;
 
 	mutex_lock(&ar->conf_mutex);
 
@@ -2925,11 +2923,6 @@ static void ath10k_configure_filter(struct ieee80211_hw *hw,
 	*total_flags &= SUPPORTED_FILTERS;
 	ar->filter_flags = *total_flags;
 
-	ar->promisc = !!(ar->filter_flags & FIF_PROMISC_IN_BSS);
-	ret = ath10k_monitor_recalc(ar);
-	if (ret)
-		ath10k_warn("failed to recalc montior: %d\n", ret);
-
 	mutex_unlock(&ar->conf_mutex);
 }
 
-- 
1.8.5.3

Re: [RFC/RFT 2/2] ath10k: don't start monitor vdev for promisc

[Ben Greear]

On 07/16/2014 06:21 AM, Michal Kazior wrote:
> ath10k doesn't apply any extra rx filters so
> there's no need to start monitor vdev for
> promiscuous mode.
>
> This fixes crashes with 4addr station interface
> bridging and some very rare crashes of AP
> interfaces with bridging as well.

Ahh, I was just working on some related hack-arounds to make sure
I left a vdev slot open for the monitor interface in case some poor
person started a sniffer on an interface and made it go promisc...

With this patch, I can be sure monitor interfaces will not
be automatically created without explicit user request?

Any idea if it will be a problem to apply this to what is
effectively a 3.15 kernel?

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [RFC/RFT 2/2] ath10k: don't start monitor vdev for promisc

[Michal Kazior]

On 16 July 2014 17:28, Ben Greear <greearb@candelatech.com> wrote:
> On 07/16/2014 06:21 AM, Michal Kazior wrote:
>>
>> ath10k doesn't apply any extra rx filters so
>> there's no need to start monitor vdev for
>> promiscuous mode.
>>
>> This fixes crashes with 4addr station interface
>> bridging and some very rare crashes of AP
>> interfaces with bridging as well.
>
>
> Ahh, I was just working on some related hack-arounds to make sure
> I left a vdev slot open for the monitor interface in case some poor
> person started a sniffer on an interface and made it go promisc...
>
> With this patch, I can be sure monitor interfaces will not
> be automatically created without explicit user request?

I'm not sure if I understand you correctly, but:

With this patch monitor vdev will be created and started only if there
is at least one monitor interface up and running or CAC is in
progress. Promiscuous mode does not change driver state at all. IOW if
you start tcpdump on a wlan0 (station iftype) or add wlan0 (ap iftype,
sta iftype) to a bridge then monitor vdev will not be created. You
have to iw wlan0 interface add mon type monitor && ip link set mon up
to start monitor vdev. From then on you can tcpdump or tcpdump -p on
mon to sniff everything that the card sees on the air.


> Any idea if it will be a problem to apply this to what is
> effectively a 3.15 kernel?

Hmm.. I guess it should apply. This depends on some earlier monitor
clean up patches but I can't remember when those were merged.


Michał

Re: [RFC/RFT 1/2] ath10k: fix monitor start/stop sequences

[Kalle Valo]

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

> Fix some cases where monitor start failure left
> the driver in a confused state.
>
> This also makes the monitor code simpler.
>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>

>From a quick review looks good to me. Are you planning to submit this as
a proper patch at some point?

Did anyone test this?

-- 
Kalle Valo

Re: [RFC/RFT 1/2] ath10k: fix monitor start/stop sequences

[Michal Kazior]

On 21 July 2014 19:39, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> Michal Kazior <michal.kazior@tieto.com> writes:
>
>> Fix some cases where monitor start failure left
>> the driver in a confused state.
>>
>> This also makes the monitor code simpler.
>>
>> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
>
> From a quick review looks good to me. Are you planning to submit this as
> a proper patch at some point?

I've been waiting for feedback in case this breaks someone's setup/use
cases (it shouldn't but hey). No one has complained so I guess I'll be
posting this as a PATCH soon.


Michał