[2/6] brcmfmac: send port authorized event for 802.1X 4-way handshake offload
diff mbox series

Message ID 1546582221-143220-2-git-send-email-chi-hsien.lin@cypress.com
State Changes Requested
Delegated to: Johannes Berg
Headers show
Series
  • [1/6] nl80211: add NL80211_ATTR_IFINDEX to port authorized event
Related show

Commit Message

Chi-Hsien Lin Jan. 4, 2019, 6:11 a.m. UTC
From: Chung-Hsien Hsu <stanley.hsu@cypress.com>

With 4-way handshake offload for 802.1X authentication, a port
authorized event should be sent to user space after the completion of
4-way handshake. It is used to indicate that a connection is authorized
and 802.1X authentication is no longer required.

Signed-off-by: Chung-Hsien Hsu <stanley.hsu@cypress.com>
Signed-off-by: Chi-Hsien Lin <chi-hsien.lin@cypress.com>
---
 .../broadcom/brcm80211/brcmfmac/cfg80211.c         | 23 +++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

Comments

Arend Van Spriel Jan. 7, 2019, 9:44 a.m. UTC | #1
On 1/4/2019 7:11 AM, Chi-Hsien Lin wrote:
> From: Chung-Hsien Hsu <stanley.hsu@cypress.com>
> 
> With 4-way handshake offload for 802.1X authentication, a port
> authorized event should be sent to user space after the completion of
> 4-way handshake. It is used to indicate that a connection is authorized
> and 802.1X authentication is no longer required.

It had been a while since I had looked at our offload code (basically 
since the initial implementation for the nl80211 work) so I was unsure 
why this would be needed.

So initially we added a PORT_AUTHORIZED *attribute* in the nl80211 api 
and later on the PORT_AUTHORIZED *event* was introduced and 4-way hs 
offload support in wpa_supplicant is ignoring the *attribute* and only 
handling the *event*. I think this information is important enough to 
add to this commit message with a reference to commit 503c1fb98ba3 
("cfg80211/nl80211: add a port authorized event") which "broke" the 
functionality in brcmfmac.

Some specific comments below...

Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Signed-off-by: Chung-Hsien Hsu <stanley.hsu@cypress.com>
> Signed-off-by: Chi-Hsien Lin <chi-hsien.lin@cypress.com>
> ---
>   .../broadcom/brcm80211/brcmfmac/cfg80211.c         | 23 +++++++++++++++-------
>   1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> index 35301237d435..ad0d775a1244 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> @@ -5266,10 +5266,13 @@ static bool brcmf_is_linkup(struct brcmf_cfg80211_vif *vif,
>   	u32 event = e->event_code;
>   	u32 status = e->status;
>   
> -	if (vif->profile.use_fwsup == BRCMF_PROFILE_FWSUP_PSK &&
> -	    event == BRCMF_E_PSK_SUP &&
> -	    status == BRCMF_E_STATUS_FWSUP_COMPLETED)
> +	if (event == BRCMF_E_PSK_SUP &&
> +	    status == BRCMF_E_STATUS_FWSUP_COMPLETED) {
>   		set_bit(BRCMF_VIF_STATUS_EAP_SUCCESS, &vif->sme_state);
> +		if (vif->profile.use_fwsup == BRCMF_PROFILE_FWSUP_1X)
> +			return true;
> +	}
> +

Here things get a bit tricky I think. The old behaviour was to wait for 
both PSK_SUP and SET_SSID events before calling cfg80211_connect_done(). 
If I recall correctly I did that because they can arrive in different 
order depending on the type of offload (1x or psk) but also depends on 
firmware build, so ....

>   	if (event == BRCMF_E_SET_SSID && status == BRCMF_E_STATUS_SUCCESS) {
>   		brcmf_dbg(CONN, "Processing set ssid\n");
>   		memcpy(vif->profile.bssid, e->addr, ETH_ALEN);
> @@ -5280,11 +5283,10 @@ static bool brcmf_is_linkup(struct brcmf_cfg80211_vif *vif,
>   	}
>   
>   	if (test_bit(BRCMF_VIF_STATUS_EAP_SUCCESS, &vif->sme_state) &&
> -	    test_bit(BRCMF_VIF_STATUS_ASSOC_SUCCESS, &vif->sme_state)) {
> -		clear_bit(BRCMF_VIF_STATUS_EAP_SUCCESS, &vif->sme_state);
> -		clear_bit(BRCMF_VIF_STATUS_ASSOC_SUCCESS, &vif->sme_state);
> +	    test_and_clear_bit(BRCMF_VIF_STATUS_ASSOC_SUCCESS,
> +			       &vif->sme_state))
>   		return true;
> -	}
> +
>   	return false;
>   }
>   
> @@ -5501,6 +5503,13 @@ brcmf_bss_connect_done(struct brcmf_cfg80211_info *cfg,
>   		brcmf_dbg(CONN, "Report connect result - connection %s\n",
>   			  completed ? "succeeded" : "failed");
>   	}
> +
> +	if (test_and_clear_bit(BRCMF_VIF_STATUS_EAP_SUCCESS,
> +			       &ifp->vif->sme_state) &&
> +	    profile->use_fwsup == BRCMF_PROFILE_FWSUP_1X) {
> +		cfg80211_port_authorized(ndev, profile->bssid, GFP_KERNEL);
> +		brcmf_dbg(CONN, "Report port authorized\n");
> +	}

I would leave the code in brcmf_is_linkup() as before and only check 
profile->use_fwsup here to determine whether cfg80211_port_authorized() 
should be called here.

>   	brcmf_dbg(TRACE, "Exit\n");
>   	return 0;
>   }
>

Patch
diff mbox series

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index 35301237d435..ad0d775a1244 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -5266,10 +5266,13 @@  static bool brcmf_is_linkup(struct brcmf_cfg80211_vif *vif,
 	u32 event = e->event_code;
 	u32 status = e->status;
 
-	if (vif->profile.use_fwsup == BRCMF_PROFILE_FWSUP_PSK &&
-	    event == BRCMF_E_PSK_SUP &&
-	    status == BRCMF_E_STATUS_FWSUP_COMPLETED)
+	if (event == BRCMF_E_PSK_SUP &&
+	    status == BRCMF_E_STATUS_FWSUP_COMPLETED) {
 		set_bit(BRCMF_VIF_STATUS_EAP_SUCCESS, &vif->sme_state);
+		if (vif->profile.use_fwsup == BRCMF_PROFILE_FWSUP_1X)
+			return true;
+	}
+
 	if (event == BRCMF_E_SET_SSID && status == BRCMF_E_STATUS_SUCCESS) {
 		brcmf_dbg(CONN, "Processing set ssid\n");
 		memcpy(vif->profile.bssid, e->addr, ETH_ALEN);
@@ -5280,11 +5283,10 @@  static bool brcmf_is_linkup(struct brcmf_cfg80211_vif *vif,
 	}
 
 	if (test_bit(BRCMF_VIF_STATUS_EAP_SUCCESS, &vif->sme_state) &&
-	    test_bit(BRCMF_VIF_STATUS_ASSOC_SUCCESS, &vif->sme_state)) {
-		clear_bit(BRCMF_VIF_STATUS_EAP_SUCCESS, &vif->sme_state);
-		clear_bit(BRCMF_VIF_STATUS_ASSOC_SUCCESS, &vif->sme_state);
+	    test_and_clear_bit(BRCMF_VIF_STATUS_ASSOC_SUCCESS,
+			       &vif->sme_state))
 		return true;
-	}
+
 	return false;
 }
 
@@ -5501,6 +5503,13 @@  brcmf_bss_connect_done(struct brcmf_cfg80211_info *cfg,
 		brcmf_dbg(CONN, "Report connect result - connection %s\n",
 			  completed ? "succeeded" : "failed");
 	}
+
+	if (test_and_clear_bit(BRCMF_VIF_STATUS_EAP_SUCCESS,
+			       &ifp->vif->sme_state) &&
+	    profile->use_fwsup == BRCMF_PROFILE_FWSUP_1X) {
+		cfg80211_port_authorized(ndev, profile->bssid, GFP_KERNEL);
+		brcmf_dbg(CONN, "Report port authorized\n");
+	}
 	brcmf_dbg(TRACE, "Exit\n");
 	return 0;
 }