+ Jithu, Eylon

On 3/29/2018 1:16 PM, Johannes Berg wrote:

Hi Arend,

quoted

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?

That's indeed a bit old :-)

quoted

quoted

quoted

+ * @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:

quoted

+	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?

[snip]

quoted

quoted

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).

Agree, and we don't even have any drivers that are setting the
FILS_SK_OFFLOAD flag anyway, so we can still redefine its semantics to
some extent.

There is some implied behavior about the UPDATE_AUTH_TYPE. The 
FILS_SK_OFFLOAD only seems to cover NL80211_AUTHTYPE_FILS_SK. So to me 
it seems that changing the auth type really means the driver should give 
up on roaming and let user-space handle it.

So yeah, I'd argue that what the patch needed was somebody taking a
critical look at my review ;-)

And perhaps fixing the weird flags thing I pointed out.

Yup. That made sense.

Also there is a DOC section about FILS shared key authentication 
offload" so I suppose that should be extended as well.

Regards,
Arend