[PATCH 1/2] wifi: cfg80211: fix CQM for non-range use

[PATCH 1/2] wifi: cfg80211: fix CQM for non-range use

[Léo Lam]

From: Johannes Berg <johannes.berg@intel.com>

commit 7e7efdda6adb385fbdfd6f819d76bc68c923c394 upstream.

[note: this is commit 4a7e92551618f3737b305f62451353ee05662f57 reapplied;
that commit had been reverted in 6.6.6 because it caused regressions, see
https://lore.kernel.org/stable/2023121450-habitual-transpose-68a1@gregkh/
for details]

My prior race fix here broke CQM when ranges aren't used, as
the reporting worker now requires the cqm_config to be set in
the wdev, but isn't set when there's no range configured.

Rather than continuing to special-case the range version, set
the cqm_config always and configure accordingly, also tracking
if range was used or not to be able to clear the configuration
appropriately with the same API, which was actually not right
if both were implemented by a driver for some reason, as is
the case with mac80211 (though there the implementations are
equivalent so it doesn't matter.)

Also, the original multiple-RSSI commit lost checking for the
callback, so might have potentially crashed if a driver had
neither implementation, and userspace tried to use it despite
not being advertised as supported.

Cc: stable@vger.kernel.org
Fixes: 4a4b8169501b ("cfg80211: Accept multiple RSSI thresholds for CQM")
Fixes: 37c20b2effe9 ("wifi: cfg80211: fix cqm_config access race")
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Léo Lam <leo@leolam.fr>
---
 net/wireless/core.h    |  1 +
 net/wireless/nl80211.c | 50 ++++++++++++++++++++++++++----------------
 2 files changed, 32 insertions(+), 19 deletions(-)

diff --git a/net/wireless/core.h b/net/wireless/core.h
index e536c0b615a0..f0a3a2317638 100644
--- a/net/wireless/core.h
+++ b/net/wireless/core.h
@@ -299,6 +299,7 @@ struct cfg80211_cqm_config {
 	u32 rssi_hyst;
 	s32 last_rssi_event_value;
 	enum nl80211_cqm_rssi_threshold_event last_rssi_event_type;
+	bool use_range_api;
 	int n_rssi_thresholds;
 	s32 rssi_thresholds[] __counted_by(n_rssi_thresholds);
 };
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 931a03f4549c..6a82dd876f27 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -12824,10 +12824,6 @@ static int cfg80211_cqm_rssi_update(struct cfg80211_registered_device *rdev,
 	int i, n, low_index;
 	int err;
 
-	/* RSSI reporting disabled? */
-	if (!cqm_config)
-		return rdev_set_cqm_rssi_range_config(rdev, dev, 0, 0);
-
 	/*
 	 * Obtain current RSSI value if possible, if not and no RSSI threshold
 	 * event has been received yet, we should receive an event after a
@@ -12902,18 +12898,6 @@ static int nl80211_set_cqm_rssi(struct genl_info *info,
 	    wdev->iftype != NL80211_IFTYPE_P2P_CLIENT)
 		return -EOPNOTSUPP;
 
-	if (n_thresholds <= 1 && rdev->ops->set_cqm_rssi_config) {
-		if (n_thresholds == 0 || thresholds[0] == 0) /* Disabling */
-			return rdev_set_cqm_rssi_config(rdev, dev, 0, 0);
-
-		return rdev_set_cqm_rssi_config(rdev, dev,
-						thresholds[0], hysteresis);
-	}
-
-	if (!wiphy_ext_feature_isset(&rdev->wiphy,
-				     NL80211_EXT_FEATURE_CQM_RSSI_LIST))
-		return -EOPNOTSUPP;
-
 	if (n_thresholds == 1 && thresholds[0] == 0) /* Disabling */
 		n_thresholds = 0;
 
@@ -12921,6 +12905,20 @@ static int nl80211_set_cqm_rssi(struct genl_info *info,
 	old = rcu_dereference_protected(wdev->cqm_config,
 					lockdep_is_held(&wdev->mtx));
 
+	/* if already disabled just succeed */
+	if (!n_thresholds && !old)
+		return 0;
+
+	if (n_thresholds > 1) {
+		if (!wiphy_ext_feature_isset(&rdev->wiphy,
+					     NL80211_EXT_FEATURE_CQM_RSSI_LIST) ||
+		    !rdev->ops->set_cqm_rssi_range_config)
+			return -EOPNOTSUPP;
+	} else {
+		if (!rdev->ops->set_cqm_rssi_config)
+			return -EOPNOTSUPP;
+	}
+
 	if (n_thresholds) {
 		cqm_config = kzalloc(struct_size(cqm_config, rssi_thresholds,
 						 n_thresholds),
@@ -12935,13 +12933,26 @@ static int nl80211_set_cqm_rssi(struct genl_info *info,
 		memcpy(cqm_config->rssi_thresholds, thresholds,
 		       flex_array_size(cqm_config, rssi_thresholds,
 				       n_thresholds));
+		cqm_config->use_range_api = n_thresholds > 1 ||
+					    !rdev->ops->set_cqm_rssi_config;
 
 		rcu_assign_pointer(wdev->cqm_config, cqm_config);
+
+		if (cqm_config->use_range_api)
+			err = cfg80211_cqm_rssi_update(rdev, dev, cqm_config);
+		else
+			err = rdev_set_cqm_rssi_config(rdev, dev,
+						       thresholds[0],
+						       hysteresis);
 	} else {
 		RCU_INIT_POINTER(wdev->cqm_config, NULL);
+		/* if enabled as range also disable via range */
+		if (old->use_range_api)
+			err = rdev_set_cqm_rssi_range_config(rdev, dev, 0, 0);
+		else
+			err = rdev_set_cqm_rssi_config(rdev, dev, 0, 0);
 	}
 
-	err = cfg80211_cqm_rssi_update(rdev, dev, cqm_config);
 	if (err) {
 		rcu_assign_pointer(wdev->cqm_config, old);
 		kfree_rcu(cqm_config, rcu_head);
@@ -19131,10 +19142,11 @@ void cfg80211_cqm_rssi_notify_work(struct wiphy *wiphy, struct wiphy_work *work)
 	wdev_lock(wdev);
 	cqm_config = rcu_dereference_protected(wdev->cqm_config,
 					       lockdep_is_held(&wdev->mtx));
-	if (!wdev->cqm_config)
+	if (!cqm_config)
 		goto unlock;
 
-	cfg80211_cqm_rssi_update(rdev, wdev->netdev, cqm_config);
+	if (cqm_config->use_range_api)
+		cfg80211_cqm_rssi_update(rdev, wdev->netdev, cqm_config);
 
 	rssi_level = cqm_config->last_rssi_event_value;
 	rssi_event = cqm_config->last_rssi_event_type;
-- 
2.43.0

[PATCH 2/2] wifi: nl80211: fix deadlock in nl80211_set_cqm_rssi (6.6.x)

[Léo Lam]

Commit 008afb9f3d57 ("wifi: cfg80211: fix CQM for non-range use"
backported to 6.6.x) causes nl80211_set_cqm_rssi not to release the
wdev lock in some of the error paths.

Of course, the ensuing deadlock causes userland network managers to
break pretty badly, and on typical systems this also causes lockups on
on suspend, poweroff and reboot. See [1], [2], [3] for example reports.

The upstream commit 7e7efdda6adb ("wifi: cfg80211: fix CQM for non-range
use"), committed in November 2023, is completely fine because there was
another commit in August 2023 that removed the wdev lock:
see commit 076fc8775daf ("wifi: cfg80211: remove wdev mutex").

The reason things broke in 6.6.5 is that commit 4338058f6009 was applied
without also applying 076fc8775daf.

Commit 076fc8775daf ("wifi: cfg80211: remove wdev mutex") is a rather
large commit; adjusting the error handling (which is what this commit does)
yields a much simpler patch and was tested to work properly.

Fix the deadlock by releasing the lock before returning.

[1] https://bugzilla.kernel.org/show_bug.cgi?id=218247
[2] https://bbs.archlinux.org/viewtopic.php?id=290976
[3] https://lore.kernel.org/all/87sf4belmm.fsf@turtle.gmx.de/

Link: https://lore.kernel.org/stable/e374bb16-5b13-44cc-b11a-2f4eefb1ecf5@manjaro.org/
Fixes: 008afb9f3d57 ("wifi: cfg80211: fix CQM for non-range use")
Tested-by: Léo Lam <leo@leolam.fr>
Tested-by: Philip Müller <philm@manjaro.org>
Cc: stable@vger.kernel.org
Cc: Johannes Berg <johannes.berg@intel.com>
Signed-off-by: Léo Lam <leo@leolam.fr>
---
 net/wireless/nl80211.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 6a82dd876f27..0b0dfecedc50 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -12906,17 +12906,23 @@ static int nl80211_set_cqm_rssi(struct genl_info *info,
 					lockdep_is_held(&wdev->mtx));
 
 	/* if already disabled just succeed */
-	if (!n_thresholds && !old)
-		return 0;
+	if (!n_thresholds && !old) {
+		err = 0;
+		goto unlock;
+	}
 
 	if (n_thresholds > 1) {
 		if (!wiphy_ext_feature_isset(&rdev->wiphy,
 					     NL80211_EXT_FEATURE_CQM_RSSI_LIST) ||
-		    !rdev->ops->set_cqm_rssi_range_config)
-			return -EOPNOTSUPP;
+		    !rdev->ops->set_cqm_rssi_range_config) {
+			err = -EOPNOTSUPP;
+			goto unlock;
+		}
 	} else {
-		if (!rdev->ops->set_cqm_rssi_config)
-			return -EOPNOTSUPP;
+		if (!rdev->ops->set_cqm_rssi_config) {
+			err = -EOPNOTSUPP;
+			goto unlock;
+		}
 	}
 
 	if (n_thresholds) {
-- 
2.43.0

Re: [PATCH 2/2] wifi: nl80211: fix deadlock in nl80211_set_cqm_rssi (6.6.x)

[Greg KH]

On Sat, Dec 16, 2023 at 05:47:17AM +0000, Léo Lam wrote:
> Commit 008afb9f3d57 ("wifi: cfg80211: fix CQM for non-range use"
> backported to 6.6.x) causes nl80211_set_cqm_rssi not to release the
> wdev lock in some of the error paths.
> 
> Of course, the ensuing deadlock causes userland network managers to
> break pretty badly, and on typical systems this also causes lockups on
> on suspend, poweroff and reboot. See [1], [2], [3] for example reports.
> 
> The upstream commit 7e7efdda6adb ("wifi: cfg80211: fix CQM for non-range
> use"), committed in November 2023, is completely fine because there was
> another commit in August 2023 that removed the wdev lock:
> see commit 076fc8775daf ("wifi: cfg80211: remove wdev mutex").
> 
> The reason things broke in 6.6.5 is that commit 4338058f6009 was applied
> without also applying 076fc8775daf.
> 
> Commit 076fc8775daf ("wifi: cfg80211: remove wdev mutex") is a rather
> large commit; adjusting the error handling (which is what this commit does)
> yields a much simpler patch and was tested to work properly.
> 
> Fix the deadlock by releasing the lock before returning.
> 
> [1] https://bugzilla.kernel.org/show_bug.cgi?id=218247
> [2] https://bbs.archlinux.org/viewtopic.php?id=290976
> [3] https://lore.kernel.org/all/87sf4belmm.fsf@turtle.gmx.de/
> 
> Link: https://lore.kernel.org/stable/e374bb16-5b13-44cc-b11a-2f4eefb1ecf5@manjaro.org/
> Fixes: 008afb9f3d57 ("wifi: cfg80211: fix CQM for non-range use")
> Tested-by: Léo Lam <leo@leolam.fr>
> Tested-by: Philip Müller <philm@manjaro.org>
> Cc: stable@vger.kernel.org
> Cc: Johannes Berg <johannes.berg@intel.com>
> Signed-off-by: Léo Lam <leo@leolam.fr>
> ---
>  net/wireless/nl80211.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)

Both now queued up, thanks.

greg k-h

Re: [PATCH 2/2] wifi: nl80211: fix deadlock in nl80211_set_cqm_rssi (6.6.x)

[Philip Müller]

On 30.12.23 18:43, Greg KH wrote:
> On Sat, Dec 16, 2023 at 05:47:17AM +0000, Léo Lam wrote:
>> Commit 008afb9f3d57 ("wifi: cfg80211: fix CQM for non-range use"
>> backported to 6.6.x) causes nl80211_set_cqm_rssi not to release the
>> wdev lock in some of the error paths.
>>
>> Of course, the ensuing deadlock causes userland network managers to
>> break pretty badly, and on typical systems this also causes lockups on
>> on suspend, poweroff and reboot. See [1], [2], [3] for example reports.
>>
>> The upstream commit 7e7efdda6adb ("wifi: cfg80211: fix CQM for non-range
>> use"), committed in November 2023, is completely fine because there was
>> another commit in August 2023 that removed the wdev lock:
>> see commit 076fc8775daf ("wifi: cfg80211: remove wdev mutex").
>>
>> The reason things broke in 6.6.5 is that commit 4338058f6009 was applied
>> without also applying 076fc8775daf.
>>
>> Commit 076fc8775daf ("wifi: cfg80211: remove wdev mutex") is a rather
>> large commit; adjusting the error handling (which is what this commit does)
>> yields a much simpler patch and was tested to work properly.
>>
>> Fix the deadlock by releasing the lock before returning.
>>
>> [1] https://bugzilla.kernel.org/show_bug.cgi?id=218247
>> [2] https://bbs.archlinux.org/viewtopic.php?id=290976
>> [3] https://lore.kernel.org/all/87sf4belmm.fsf@turtle.gmx.de/
>>
>> Link: https://lore.kernel.org/stable/e374bb16-5b13-44cc-b11a-2f4eefb1ecf5@manjaro.org/
>> Fixes: 008afb9f3d57 ("wifi: cfg80211: fix CQM for non-range use")
>> Tested-by: Léo Lam <leo@leolam.fr>
>> Tested-by: Philip Müller <philm@manjaro.org>
>> Cc: stable@vger.kernel.org
>> Cc: Johannes Berg <johannes.berg@intel.com>
>> Signed-off-by: Léo Lam <leo@leolam.fr>
>> ---
>>   net/wireless/nl80211.c | 18 ++++++++++++------
>>   1 file changed, 12 insertions(+), 6 deletions(-)
> 
> Both now queued up, thanks.
> 
> greg k-h

Hi Greg,

seems only for 6.1.x series. Still don't see it for 6.6.x ...
-- 
Best, Philip