diff mbox series

[v2,6/9] netdev: add netdev_ft_reassociate

Message ID 20231206150708.2080336-7-prestwoj@gmail.com (mailing list archive)
State New
Headers show
Series Reassoc/FT roaming unification | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
prestwoj/iwd-ci-gitlint success GitLint

Commit Message

James Prestwood Dec. 6, 2023, 3:07 p.m. UTC 
Essentially exposes (and renames) netdev_ft_tx_associate in order to
be called similarly to netdev_reassociate/netdev_connect where a
connect callback can be provided. This will fix the current bug where
if association times out during FT IWD will hang and never transition
to disconnected.

This also removes the calling of the FT_ROAMED event and instead just
calls the connect callback (since its now set). This unifies the
callback path for reassociation and FT roaming.
---
 src/netdev.c | 44 ++++++++++++++++++++++++++------------------
 src/netdev.h |  5 +++++
 2 files changed, 31 insertions(+), 18 deletions(-)

Comments

Denis Kenzior Dec. 6, 2023, 4:40 p.m. UTC | #1 
Hi James,

On 12/6/23 09:07, James Prestwood wrote:
> Essentially exposes (and renames) netdev_ft_tx_associate in order to
> be called similarly to netdev_reassociate/netdev_connect where a
> connect callback can be provided. This will fix the current bug where
> if association times out during FT IWD will hang and never transition
> to disconnected.
> 

I like it!

Nitpick below...

> This also removes the calling of the FT_ROAMED event and instead just
> calls the connect callback (since its now set). This unifies the
> callback path for reassociation and FT roaming.
> ---
>   src/netdev.c | 44 ++++++++++++++++++++++++++------------------
>   src/netdev.h |  5 +++++
>   2 files changed, 31 insertions(+), 18 deletions(-)
> 

<snip>

> diff --git a/src/netdev.c b/src/netdev.c
> index f2e887b4..7d52ffea 100644
> --- a/src/netdev.c
> +++ b/src/netdev.c
> @@ -1409,16 +1409,14 @@ static void netdev_connect_ok(struct netdev *netdev)
>   			scan_bss_free(netdev->fw_roam_bss);
>   
>   		netdev->fw_roam_bss = NULL;
> -	} else if (netdev->in_ft) {
> -		if (netdev->event_filter)
> -			netdev->event_filter(netdev, NETDEV_EVENT_FT_ROAMED,
> -						NULL, netdev->user_data);
> -		netdev->in_ft = false;
>   	} else if (netdev->connect_cb) {
>   		netdev->connect_cb(netdev, NETDEV_RESULT_OK, NULL,
>   					netdev->user_data);
>   		netdev->connect_cb = NULL;
> -	}
> +		netdev->in_ft = false;
> +		netdev->in_reassoc = false;

Why do we set in_reassoc to false here?  Shouldn't it be taken care of already 
by the associate_event?

> +	} else
> +		l_warn("Connection event without a connect callback!");
>   
>   	netdev_rssi_polling_update(netdev);
>   

<snip>

> @@ -6256,7 +6265,6 @@ static int netdev_init(void)
>   	__eapol_set_install_pmk_func(netdev_set_pmk);
>   
>   	__ft_set_tx_frame_func(netdev_tx_ft_frame);
> -	__ft_set_tx_associate_func(netdev_ft_tx_associate);

This part probably belongs in the next patch or even patch 8.

>   
>   	unicast_watch = l_genl_add_unicast_watch(genl, NL80211_GENL_NAME,
>   						netdev_unicast_notify,

Regards,
-Denis
James Prestwood Dec. 6, 2023, 4:49 p.m. UTC | #2 
On 12/6/23 08:40, Denis Kenzior wrote:
> Hi James,
>
> On 12/6/23 09:07, James Prestwood wrote:
>> Essentially exposes (and renames) netdev_ft_tx_associate in order to
>> be called similarly to netdev_reassociate/netdev_connect where a
>> connect callback can be provided. This will fix the current bug where
>> if association times out during FT IWD will hang and never transition
>> to disconnected.
>>
>
> I like it!
>
> Nitpick below...
>
>> This also removes the calling of the FT_ROAMED event and instead just
>> calls the connect callback (since its now set). This unifies the
>> callback path for reassociation and FT roaming.
>> ---
>>   src/netdev.c | 44 ++++++++++++++++++++++++++------------------
>>   src/netdev.h |  5 +++++
>>   2 files changed, 31 insertions(+), 18 deletions(-)
>>
>
> <snip>
>
>> diff --git a/src/netdev.c b/src/netdev.c
>> index f2e887b4..7d52ffea 100644
>> --- a/src/netdev.c
>> +++ b/src/netdev.c
>> @@ -1409,16 +1409,14 @@ static void netdev_connect_ok(struct netdev 
>> *netdev)
>>               scan_bss_free(netdev->fw_roam_bss);
>>             netdev->fw_roam_bss = NULL;
>> -    } else if (netdev->in_ft) {
>> -        if (netdev->event_filter)
>> -            netdev->event_filter(netdev, NETDEV_EVENT_FT_ROAMED,
>> -                        NULL, netdev->user_data);
>> -        netdev->in_ft = false;
>>       } else if (netdev->connect_cb) {
>>           netdev->connect_cb(netdev, NETDEV_RESULT_OK, NULL,
>>                       netdev->user_data);
>>           netdev->connect_cb = NULL;
>> -    }
>> +        netdev->in_ft = false;
>> +        netdev->in_reassoc = false;
>
> Why do we set in_reassoc to false here?  Shouldn't it be taken care of 
> already by the associate_event?
I did this out of an abundance of caution. But yes I see we set it to 
false if !netdev->ap && !netdev->in_ft, so I can remove this.
>
>> +    } else
>> +        l_warn("Connection event without a connect callback!");
>>         netdev_rssi_polling_update(netdev);
>
> <snip>
>
>> @@ -6256,7 +6265,6 @@ static int netdev_init(void)
>>       __eapol_set_install_pmk_func(netdev_set_pmk);
>>         __ft_set_tx_frame_func(netdev_tx_ft_frame);
>> -    __ft_set_tx_associate_func(netdev_ft_tx_associate);
>
> This part probably belongs in the next patch or even patch 8.

The problem is I removed the function so gcc will warn/break the build.

>
>>         unicast_watch = l_genl_add_unicast_watch(genl, 
>> NL80211_GENL_NAME,
>>                           netdev_unicast_notify,
>
> Regards,
> -Denis
>
diff mbox series

Patch

diff --git a/src/netdev.c b/src/netdev.c
index f2e887b4..7d52ffea 100644
--- a/src/netdev.c
+++ b/src/netdev.c
@@ -1409,16 +1409,14 @@  static void netdev_connect_ok(struct netdev *netdev)
 			scan_bss_free(netdev->fw_roam_bss);
 
 		netdev->fw_roam_bss = NULL;
-	} else if (netdev->in_ft) {
-		if (netdev->event_filter)
-			netdev->event_filter(netdev, NETDEV_EVENT_FT_ROAMED,
-						NULL, netdev->user_data);
-		netdev->in_ft = false;
 	} else if (netdev->connect_cb) {
 		netdev->connect_cb(netdev, NETDEV_RESULT_OK, NULL,
 					netdev->user_data);
 		netdev->connect_cb = NULL;
-	}
+		netdev->in_ft = false;
+		netdev->in_reassoc = false;
+	} else
+		l_warn("Connection event without a connect callback!");
 
 	netdev_rssi_polling_update(netdev);
 
@@ -4205,13 +4203,14 @@  static int netdev_tx_ft_frame(uint32_t ifindex, uint16_t frame_type,
 	return 0;
 }
 
-static int netdev_ft_tx_associate(uint32_t ifindex, uint32_t freq,
-					const uint8_t *prev_bssid,
-					struct iovec *ft_iov, size_t n_ft_iov)
+int netdev_ft_reassociate(struct netdev *netdev,
+				const struct scan_bss *target_bss,
+				const struct scan_bss *orig_bss,
+				netdev_event_func_t event_filter,
+				netdev_connect_cb_t cb, void *user_data)
 {
-	struct netdev *netdev = netdev_find(ifindex);
-	struct netdev_handshake_state *nhs;
 	struct handshake_state *hs = netdev->handshake;
+	struct netdev_handshake_state *nhs;
 	struct l_genl_msg *msg;
 	struct iovec iov[64];
 	unsigned int n_iov = L_ARRAY_SIZE(iov);
@@ -4223,11 +4222,14 @@  static int netdev_ft_tx_associate(uint32_t ifindex, uint32_t freq,
 	 * At this point there is no going back with FT so reset all the flags
 	 * needed to associate with a new BSS.
 	 */
-	netdev->frequency = freq;
+	netdev->frequency = target_bss->frequency;
 	netdev->handshake->active_tk_index = 0;
 	netdev->associated = false;
 	netdev->operational = false;
 	netdev->in_ft = true;
+	netdev->event_filter = event_filter;
+	netdev->connect_cb = cb;
+	netdev->user_data = user_data;
 
 	/*
 	 * Cancel commands that could be running because of EAPoL activity
@@ -4271,15 +4273,22 @@  static int netdev_ft_tx_associate(uint32_t ifindex, uint32_t freq,
 
 	c_iov = netdev_populate_common_ies(netdev, hs, msg, iov, n_iov, c_iov);
 
-	if (!L_WARN_ON(n_iov - c_iov < n_ft_iov)) {
-		memcpy(iov + c_iov, ft_iov, sizeof(*ft_iov) * n_ft_iov);
-		c_iov += n_ft_iov;
-	}
+	if (hs->supplicant_ie)
+		c_iov = iov_ie_append(iov, n_iov, c_iov, hs->supplicant_ie,
+					IE_LEN(hs->supplicant_ie));
+
+	if (hs->fte)
+		c_iov = iov_ie_append(iov, n_iov, c_iov, hs->fte,
+					IE_LEN(hs->fte));
+
+	if (hs->mde)
+		c_iov = iov_ie_append(iov, n_iov, c_iov, hs->mde,
+					IE_LEN(hs->mde));
 
 	mpdu_sort_ies(subtype, iov, c_iov);
 
 	l_genl_msg_append_attr(msg, NL80211_ATTR_PREV_BSSID, ETH_ALEN,
-				prev_bssid);
+				orig_bss->addr);
 	l_genl_msg_append_attrv(msg, NL80211_ATTR_IE, iov, c_iov);
 
 	netdev->connect_cmd_id = l_genl_family_send(nl80211, msg,
@@ -6256,7 +6265,6 @@  static int netdev_init(void)
 	__eapol_set_install_pmk_func(netdev_set_pmk);
 
 	__ft_set_tx_frame_func(netdev_tx_ft_frame);
-	__ft_set_tx_associate_func(netdev_ft_tx_associate);
 
 	unicast_watch = l_genl_add_unicast_watch(genl, NL80211_GENL_NAME,
 						netdev_unicast_notify,
diff --git a/src/netdev.h b/src/netdev.h
index 03d1b6e9..fb31b571 100644
--- a/src/netdev.h
+++ b/src/netdev.h
@@ -165,6 +165,11 @@  int netdev_reassociate(struct netdev *netdev,
 			struct handshake_state *hs,
 			netdev_event_func_t event_filter,
 			netdev_connect_cb_t cb, void *user_data);
+int netdev_ft_reassociate(struct netdev *netdev,
+				const struct scan_bss *target_bss,
+				const struct scan_bss *orig_bss,
+				netdev_event_func_t event_filter,
+				netdev_connect_cb_t cb, void *user_data);
 
 int netdev_preauthenticate(struct netdev *netdev,
 				const struct scan_bss *target_bss,