Re: [PATCH 6.6 074/134] wifi: cfg80211: fix CQM for non-range use

[Jaron Kent-Dobias <jaron@kent-dobias.com>]

On Saturday, 9 December 2023 at 11:05 (+0100), Sven Joachim wrote:
>On 2023-12-05 12:15 +0900, Greg Kroah-Hartman wrote:
>
>> 6.6-stable review patch.  If anyone has any objections, please let me know.
>>
>> From: Johannes Berg <johannes.berg@intel.com>
>>
>> commit 7e7efdda6adb385fbdfd6f819d76bc68c923c394 upstream.
>>
>> 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>
>> ---
>>  net/wireless/core.h    |    1
>>  net/wireless/nl80211.c |   50 ++++++++++++++++++++++++++++++-------------------
>>  2 files changed, 32 insertions(+), 19 deletions(-)
>>
>> --- 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);
>>  };
>> --- a/net/wireless/nl80211.c
>> +++ b/net/wireless/nl80211.c
>> @@ -12824,10 +12824,6 @@ static int cfg80211_cqm_rssi_update(stru
>>  	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 g
>>  	    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 g
>>  	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 g
>>  		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(struc
>>  	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;
>
>After upgrading to 6.6.5, I noticed that my laptop would hang on
>shutdown and bisected that problem to this patch.  Reverting it makes
>the problem go away.
>
>More specifically, NetworkManager and wpa_supplicant processes are hung.
>This can also be triggered by "systemctl stop NetworkManager.service"
>which does not complete and brings these two processes into a state of
>uninterruptible sleep.

I have a similar problem that I also traced to this commit. When I use 
6.6.5 with iwd and the proprietary broadcom-wl driver, iwd deadlocks 
when trying to associate with an access point, and eventually every 
process that tries to query network information (ip, systemd, etc) 
likewise deadlocks.

>I also tried 6.1 and mainline, and could reproduce the hang in 6.1.66
>but _not_ in 6.7-rc4, so maybe some patch from Linus' tree needs to be
>applied to the stable branches.

The problem goes away when I revert this commit on top of 6.6.5. I 
haven't tried Linus' tree or the older stable branches.

Best,
Jaron