Re: [PATCH] nl80211: Update ERP info using NL80211_CMD_UPDATE_CONNECT_PARAMS

[Arend van Spriel <arend.vanspriel@broadcom.com>]

Hi Johannes,

Picking up a somewhat old thread as I did not see a follow-up on this 
patch. I got queried about it over here by our FILS team. So what is 
needed for this patch to pass the bar?

On 12/11/2017 12:12 PM, Johannes Berg wrote:
> On Wed, 2017-10-25 at 14:50 +0530, Vidyullatha Kanchanapally wrote:
>
>> + * @UPDATE_FILS_ERP_INFO: Indicates that FILS connection parameters (realm,
>> + *	username, erp sequence number and rrk) are updated
>> + * @UPDATE_AUTH_TYPE: Indicates that Authentication type is updated
>
> These are new here, but you don't know if they were actually supported:
>
>> +	if (wiphy_ext_feature_isset(&rdev->wiphy,
>> +				    NL80211_EXT_FEATURE_FILS_SK_OFFLOAD) &&
>
> here.

The description of the FILS_SK_OFFLOAD currently says:

  * @NL80211_EXT_FEATURE_FILS_SK_OFFLOAD: Driver SME supports FILS 
shared key
  *      authentication with %NL80211_CMD_CONNECT.

Are you suggesting a new flag to cover the new update attributes?

Drivers reporting FILS_SK_OFFLOAD *and* WIPHY_FLAG_SUPPORTS_FW_ROAM 
really need this info to have any luck roaming.

>> +	    info->attrs[NL80211_ATTR_FILS_ERP_USERNAME] &&
>> +	    info->attrs[NL80211_ATTR_FILS_ERP_REALM] &&
>> +	    info->attrs[NL80211_ATTR_FILS_ERP_NEXT_SEQ_NUM] &&
>> +	    info->attrs[NL80211_ATTR_FILS_ERP_RRK]) {
> [...]
>> +	} else if (info->attrs[NL80211_ATTR_FILS_ERP_USERNAME] ||
>> +		   info->attrs[NL80211_ATTR_FILS_ERP_REALM] ||
>> +		   info->attrs[NL80211_ATTR_FILS_ERP_NEXT_SEQ_NUM] ||
>> +		   info->attrs[NL80211_ATTR_FILS_ERP_RRK]) {
>> +		return -EINVAL;
>> +	}
>
> This logic is also really odd, why not
>
> if (attrs) {
> 	if (not flag)
> 		return -EINVAL;
> 	/* use attrs etc. */
> }
>
>> +
>> +	if (info->attrs[NL80211_ATTR_AUTH_TYPE]) {
>> +		u32 auth_type =
>> +			nla_get_u32(info->attrs[NL80211_ATTR_AUTH_TYPE]);
>> +		if (!nl80211_valid_auth_type(rdev, auth_type,
>> +					     NL80211_CMD_CONNECT))
>> +			return -EINVAL;
>> +		connect.auth_type = auth_type;
>> +		changed |= UPDATE_AUTH_TYPE;
>> +	}
>
> Again, how do you know the driver will actually look at
> UPDATE_AUTH_TYPE?

If they don't they are broken, right? And if they are broken, the 
connection will drop and regular connect will happen anyway, no?

We could add a new flag to signal driver will handle the extra 
parameters in UPDATE_CONNECT_PARAMS, but it is not clear why it would be 
needed. Seems to me user-space has all the info needed with the existing 
flag(s).

Regards,
Arend