| Message ID | 20230822152931.276136-2-prestwoj@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Support fallback if FT fails | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | fail | error: patch failed: src/ft.c:58 error: src/ft.c: patch does not apply hint: Use 'git am --show-current-patch' to see the failed patch |
On 8/22/23 8:29 AM, James Prestwood wrote: > Certain return codes, though failures, can indicate that the AP is > just confused or booting up and treating it as a full failure may > not be the best route. > > For example in some production deployments if an AP is rebooted it > may take some time for neighboring APs to exchange keys for > current associations. If a client roams during that time it will > reject saying the PMKID is invalid. > > Use the ft_associate call to relay this information to station to > handle it rather than acting like there was no response. For now > this is being hard coded to status=53, but more could be added, > or the status itself could be returned if there are other specific > status codes that need to be handled. This part is wrong, I instead made ft_associate return the status directly, and station can sort out how it wants to proceed. > --- > src/ft.c | 37 ++++++++++++++++++++++++++++--------- > 1 file changed, 28 insertions(+), 9 deletions(-) > > diff --git a/src/ft.c b/src/ft.c > index c51a1288..8dfc1cb7 100644 > --- a/src/ft.c > +++ b/src/ft.c > @@ -58,6 +58,7 @@ struct ft_info { > uint32_t frequency; > uint32_t ds_frequency; > uint32_t offchannel_id; > + uint16_t status; > > struct l_timeout *timeout; > struct wiphy_radio_work_item work; > @@ -155,6 +156,7 @@ static bool ft_parse_authentication_resp_frame(const uint8_t *data, size_t len, > if (memcmp(data + 16, addr3, 6)) > return false; > > + > /* Check Authentication algorithm number is FT (2) */ > if (l_get_le16(data + 24) != 2) > return false; > @@ -527,8 +529,6 @@ static int ft_over_ds_parse_action_response(const uint8_t *frame, > return -EINVAL; > > status = l_get_le16(frame + 14); > - if (status != 0) > - return (int)status; > > if (spa_out) > *spa_out = spa; > @@ -541,7 +541,7 @@ static int ft_over_ds_parse_action_response(const uint8_t *frame, > *ies_len = frame_len - 16; > } > > - return 0; > + return (int)status; > } > > int __ft_rx_associate(uint32_t ifindex, const uint8_t *frame, size_t frame_len) > @@ -825,7 +825,7 @@ void __ft_rx_action(uint32_t ifindex, const uint8_t *frame, size_t frame_len) > > ret = ft_over_ds_parse_action_response(frame, frame_len, &spa, &aa, > &ies, &ies_len); > - if (ret != 0) { > + if (ret < 0) { > l_debug("Could not parse action response"); > return; > } > @@ -836,6 +836,14 @@ void __ft_rx_action(uint32_t ifindex, const uint8_t *frame, size_t frame_len) > return; > } > > + info->status = ret; > + > + if (info->status != 0) { > + l_debug("BSS "MAC" rejected FT action with status=%u", > + MAC_STR(info->aa), info->status); > + goto done; > + } > + > if (!ft_parse_ies(info, hs, ies, ies_len)) { > l_debug("Could not parse action response IEs"); > goto ft_error; > @@ -843,6 +851,7 @@ void __ft_rx_action(uint32_t ifindex, const uint8_t *frame, size_t frame_len) > > info->parsed = true; > > +done: > l_timeout_remove(info->timeout); > info->timeout = NULL; > > @@ -872,6 +881,7 @@ static struct ft_info *ft_info_new(struct handshake_state *hs, > target_bss->rsne[1] + 2); > > l_getrandom(info->snonce, 32); > + info->status = 0xffff; > > return info; > } > @@ -998,7 +1008,6 @@ void __ft_rx_authenticate(uint32_t ifindex, const uint8_t *frame, > struct netdev *netdev = netdev_find(ifindex); > struct handshake_state *hs = netdev_get_handshake(netdev); > struct ft_info *info; > - uint16_t status; > const uint8_t *ies; > size_t ies_len; > > @@ -1008,14 +1017,14 @@ void __ft_rx_authenticate(uint32_t ifindex, const uint8_t *frame, > > if (!ft_parse_authentication_resp_frame(frame, frame_len, > info->spa, info->aa, info->aa, 2, > - &status, &ies, &ies_len)) { > + &info->status, &ies, &ies_len)) { > l_debug("Could not parse auth response"); > return; > } > > - if (status != 0) { > + if (info->status != 0) { > l_debug("BSS "MAC" rejected FT auth with status=%u", > - MAC_STR(info->aa), status); > + MAC_STR(info->aa), info->status); > goto cancel; > } > > @@ -1165,10 +1174,20 @@ int ft_associate(uint32_t ifindex, const uint8_t *addr) > * attempt so clear out the entry so FT-over-Air can try again. > */ > if (!info->parsed) { > + uint16_t status = info->status; > + > l_queue_remove(info_list, info); > ft_info_destroy(info); > > - return -ENOENT; > + /* > + * The status may have been successful but the IEs were invalid, > + * treat this the same as no response. > + */ > + if (status == 0xffff || status == 0) > + return -ENOENT; > + > + /* If the AP rejected for some reason, relay this to station */ > + return (int)status; > } > > ft_prepare_handshake(info, hs);
Hi James, On 8/22/23 10:29, James Prestwood wrote: > Certain return codes, though failures, can indicate that the AP is > just confused or booting up and treating it as a full failure may > not be the best route. > > For example in some production deployments if an AP is rebooted it > may take some time for neighboring APs to exchange keys for > current associations. If a client roams during that time it will > reject saying the PMKID is invalid. > > Use the ft_associate call to relay this information to station to > handle it rather than acting like there was no response. For now > this is being hard coded to status=53, but more could be added, > or the status itself could be returned if there are other specific > status codes that need to be handled. Please update the description per reply to this thread. > --- > src/ft.c | 37 ++++++++++++++++++++++++++++--------- > 1 file changed, 28 insertions(+), 9 deletions(-) > > diff --git a/src/ft.c b/src/ft.c > index c51a1288..8dfc1cb7 100644 > --- a/src/ft.c > +++ b/src/ft.c > @@ -58,6 +58,7 @@ struct ft_info { > uint32_t frequency; > uint32_t ds_frequency; > uint32_t offchannel_id; > + uint16_t status; Might want to indicate that this status is for the first step (authenticate/action) > > struct l_timeout *timeout; > struct wiphy_radio_work_item work; > @@ -155,6 +156,7 @@ static bool ft_parse_authentication_resp_frame(const uint8_t *data, size_t len, > if (memcmp(data + 16, addr3, 6)) > return false; > > + This is spurious? > /* Check Authentication algorithm number is FT (2) */ > if (l_get_le16(data + 24) != 2) > return false; <snip> > @@ -836,6 +836,14 @@ void __ft_rx_action(uint32_t ifindex, const uint8_t *frame, size_t frame_len) > return; > } > > + info->status = ret; > + > + if (info->status != 0) { > + l_debug("BSS "MAC" rejected FT action with status=%u", > + MAC_STR(info->aa), info->status); > + goto done; > + } > + > if (!ft_parse_ies(info, hs, ies, ies_len)) { > l_debug("Could not parse action response IEs"); > goto ft_error; Should the status be set only once ft_parse_ies succeeded? > @@ -843,6 +851,7 @@ void __ft_rx_action(uint32_t ifindex, const uint8_t *frame, size_t frame_len) > > info->parsed = true; > > +done: > l_timeout_remove(info->timeout); > info->timeout = NULL; > > @@ -872,6 +881,7 @@ static struct ft_info *ft_info_new(struct handshake_state *hs, > target_bss->rsne[1] + 2); > > l_getrandom(info->snonce, 32); > + info->status = 0xffff; Why is this needed? > > return info; > } <snip> > @@ -1165,10 +1174,20 @@ int ft_associate(uint32_t ifindex, const uint8_t *addr) > * attempt so clear out the entry so FT-over-Air can try again. > */ > if (!info->parsed) { > + uint16_t status = info->status; > + > l_queue_remove(info_list, info); > ft_info_destroy(info); > > - return -ENOENT; > + /* > + * The status may have been successful but the IEs were invalid, > + * treat this the same as no response. > + */ > + if (status == 0xffff || status == 0) > + return -ENOENT; > + > + /* If the AP rejected for some reason, relay this to station */ > + return (int)status; I think this entire step can be dropped if you make sure status == 0 is set only if parsing succeeded. > } > > ft_prepare_handshake(info, hs); Regards, -Denis
diff --git a/src/ft.c b/src/ft.c index c51a1288..8dfc1cb7 100644 --- a/src/ft.c +++ b/src/ft.c @@ -58,6 +58,7 @@ struct ft_info { uint32_t frequency; uint32_t ds_frequency; uint32_t offchannel_id; + uint16_t status; struct l_timeout *timeout; struct wiphy_radio_work_item work; @@ -155,6 +156,7 @@ static bool ft_parse_authentication_resp_frame(const uint8_t *data, size_t len, if (memcmp(data + 16, addr3, 6)) return false; + /* Check Authentication algorithm number is FT (2) */ if (l_get_le16(data + 24) != 2) return false; @@ -527,8 +529,6 @@ static int ft_over_ds_parse_action_response(const uint8_t *frame, return -EINVAL; status = l_get_le16(frame + 14); - if (status != 0) - return (int)status; if (spa_out) *spa_out = spa; @@ -541,7 +541,7 @@ static int ft_over_ds_parse_action_response(const uint8_t *frame, *ies_len = frame_len - 16; } - return 0; + return (int)status; } int __ft_rx_associate(uint32_t ifindex, const uint8_t *frame, size_t frame_len) @@ -825,7 +825,7 @@ void __ft_rx_action(uint32_t ifindex, const uint8_t *frame, size_t frame_len) ret = ft_over_ds_parse_action_response(frame, frame_len, &spa, &aa, &ies, &ies_len); - if (ret != 0) { + if (ret < 0) { l_debug("Could not parse action response"); return; } @@ -836,6 +836,14 @@ void __ft_rx_action(uint32_t ifindex, const uint8_t *frame, size_t frame_len) return; } + info->status = ret; + + if (info->status != 0) { + l_debug("BSS "MAC" rejected FT action with status=%u", + MAC_STR(info->aa), info->status); + goto done; + } + if (!ft_parse_ies(info, hs, ies, ies_len)) { l_debug("Could not parse action response IEs"); goto ft_error; @@ -843,6 +851,7 @@ void __ft_rx_action(uint32_t ifindex, const uint8_t *frame, size_t frame_len) info->parsed = true; +done: l_timeout_remove(info->timeout); info->timeout = NULL; @@ -872,6 +881,7 @@ static struct ft_info *ft_info_new(struct handshake_state *hs, target_bss->rsne[1] + 2); l_getrandom(info->snonce, 32); + info->status = 0xffff; return info; } @@ -998,7 +1008,6 @@ void __ft_rx_authenticate(uint32_t ifindex, const uint8_t *frame, struct netdev *netdev = netdev_find(ifindex); struct handshake_state *hs = netdev_get_handshake(netdev); struct ft_info *info; - uint16_t status; const uint8_t *ies; size_t ies_len; @@ -1008,14 +1017,14 @@ void __ft_rx_authenticate(uint32_t ifindex, const uint8_t *frame, if (!ft_parse_authentication_resp_frame(frame, frame_len, info->spa, info->aa, info->aa, 2, - &status, &ies, &ies_len)) { + &info->status, &ies, &ies_len)) { l_debug("Could not parse auth response"); return; } - if (status != 0) { + if (info->status != 0) { l_debug("BSS "MAC" rejected FT auth with status=%u", - MAC_STR(info->aa), status); + MAC_STR(info->aa), info->status); goto cancel; } @@ -1165,10 +1174,20 @@ int ft_associate(uint32_t ifindex, const uint8_t *addr) * attempt so clear out the entry so FT-over-Air can try again. */ if (!info->parsed) { + uint16_t status = info->status; + l_queue_remove(info_list, info); ft_info_destroy(info); - return -ENOENT; + /* + * The status may have been successful but the IEs were invalid, + * treat this the same as no response. + */ + if (status == 0xffff || status == 0) + return -ENOENT; + + /* If the AP rejected for some reason, relay this to station */ + return (int)status; } ft_prepare_handshake(info, hs);