| Message ID | 20231115000547.1139157-3-denkenz@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] netdev: Remove improper use of netdev_connect_failed | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| prestwoj/iwd-ci-gitlint | success | GitLint |
Hi Denis, On 11/14/23 16:05, Denis Kenzior wrote: > Commit c59669a366c5 ("netdev: disambiguate between disconnection types") > introduced different paths for different types of disconnection > notifications from netdev. Formalize this further by having > netdev_connect_failed only invoke connect_cb. > > Disconnections that could be triggered outside of connection > related events are now handled on a different code path. For this > purpose, netdev_disconnected() is introduced. > --- > src/netdev.c | 45 +++++++++++++++++++++++++++++++++------------ > 1 file changed, 33 insertions(+), 12 deletions(-) LGTM. You may have already thought of this, but I do still think we could have a case where an AP could send a deauth during key setting right?. So key setting could still succeed but station would get notified of the failure via the netdev event (in netdev_disconnect_event), not the connect callback. > diff --git a/src/netdev.c b/src/netdev.c > index 327be768d3d9..e31a51617671 100644 > --- a/src/netdev.c > +++ b/src/netdev.c > @@ -839,7 +839,6 @@ static void netdev_connect_failed(struct netdev *netdev, > uint16_t status_or_reason) > { > netdev_connect_cb_t connect_cb = netdev->connect_cb; > - netdev_event_func_t event_filter = netdev->event_filter; > void *connect_data = netdev->user_data; > > /* Done this way to allow re-entrant netdev_connect calls */ > @@ -847,15 +846,6 @@ static void netdev_connect_failed(struct netdev *netdev, > > if (connect_cb) > connect_cb(netdev, result, &status_or_reason, connect_data); > - else if (event_filter) { > - /* NETDEV_EVENT_DISCONNECT_BY_SME expects a reason code */ > - if (result != NETDEV_RESULT_HANDSHAKE_FAILED) > - status_or_reason = MMPDU_REASON_CODE_UNSPECIFIED; > - > - event_filter(netdev, NETDEV_EVENT_DISCONNECT_BY_SME, > - &status_or_reason, > - connect_data); > - } > } > > static void netdev_connect_failed_cb(struct l_genl_msg *msg, void *user_data) > @@ -900,12 +890,42 @@ static void netdev_deauth_and_fail_connection(struct netdev *netdev, > netdev_send_and_fail_connection(netdev, result, status_code, msg); > } > > +/* > + * If we have a connection callback pending, either through netdev_connect > + * or netdev_reassociate, then invoke that callback with the @result and > + * @status_or_reason. Otherwise, invoke the event callback with the @event > + * and @status_or_reason. > + * > + * This is useful for situations where handshaking or setting keys somehow > + * fails (perhaps due to rekeying), or if the device is removed / brought > + * down when keys are being set as a result of a rekey > + */ > +static void netdev_disconnected(struct netdev *netdev, > + enum netdev_result result, > + enum netdev_event event, > + uint16_t status_or_reason) > +{ > + netdev_event_func_t event_filter = netdev->event_filter; > + void *event_data = netdev->user_data; > + > + if (netdev->connect_cb) { > + netdev_connect_failed(netdev, result, status_or_reason); > + return; > + } > + > + netdev_connect_free(netdev); > + > + if (event_filter) > + event_filter(netdev, event, &status_or_reason, event_data); > +} > + > static void netdev_disconnect_by_sme_cb(struct l_genl_msg *msg, void *user_data) > { > struct netdev *netdev = user_data; > > netdev->disconnect_cmd_id = 0; > - netdev_connect_failed(netdev, netdev->result, netdev->last_code); > + netdev_disconnected(netdev, netdev->result, > + NETDEV_EVENT_DISCONNECT_BY_SME, netdev->last_code); > } > > static void netdev_disconnect_by_sme(struct netdev *netdev, > @@ -1440,7 +1460,8 @@ static void netdev_setting_keys_failed(struct netdev_handshake_state *nhs, > * CMD_DISCONNECT > */ > if (err == -ENETDOWN) { > - netdev_connect_failed(netdev, NETDEV_RESULT_ABORTED, > + netdev_disconnected(netdev, NETDEV_RESULT_ABORTED, > + NETDEV_EVENT_DISCONNECT_BY_SME, > MMPDU_STATUS_CODE_UNSPECIFIED); > return; > }
Hi James, On 11/15/23 12:27, James Prestwood wrote: > Hi Denis, > > On 11/14/23 16:05, Denis Kenzior wrote: >> Commit c59669a366c5 ("netdev: disambiguate between disconnection types") >> introduced different paths for different types of disconnection >> notifications from netdev. Formalize this further by having >> netdev_connect_failed only invoke connect_cb. >> >> Disconnections that could be triggered outside of connection >> related events are now handled on a different code path. For this >> purpose, netdev_disconnected() is introduced. >> --- >> src/netdev.c | 45 +++++++++++++++++++++++++++++++++------------ >> 1 file changed, 33 insertions(+), 12 deletions(-) > > LGTM. You may have already thought of this, but I do still think we could have a > case where an AP could send a deauth during key setting right?. So key setting > could still succeed but station would get notified of the failure via the netdev > event (in netdev_disconnect_event), not the connect callback. > Yes, it can happen at any time. We have some inconsistencies in how we handle the corner cases. For example: - We Authenticate successfully, but AP Deauthenticates us before we Associate. This is handled in netdev_deauthenticate_event. See 2bebb4bdc7ee ("netdev: Handle deauth frames prior to association"). We treat this as a connect failure, but I'm no longer sure that is right. - We associate and handshake successfully, but AP decides to deauthenticate us prior to setting keys, or setting station. Pretty strange case, but should our connection throttling logic apply? Today we treat this case as a connection failure for initial connections, and don't handle it at all for the roaming case. This is what is happening in Alvin's case. - Firmware / kernel can deauthenticate us at any point. Kernel probably doesn't actually do this, but the firmware can? How should we handle these? Also, I think we might still have some problems when we connect / roam successfully but get disconnected prior to netconfig finishing. We should audit the code and make sure we're consistent. For example, are d-bus method returns and state transitions generated at the right time? Regards, -Denis
diff --git a/src/netdev.c b/src/netdev.c index 327be768d3d9..e31a51617671 100644 --- a/src/netdev.c +++ b/src/netdev.c @@ -839,7 +839,6 @@ static void netdev_connect_failed(struct netdev *netdev, uint16_t status_or_reason) { netdev_connect_cb_t connect_cb = netdev->connect_cb; - netdev_event_func_t event_filter = netdev->event_filter; void *connect_data = netdev->user_data; /* Done this way to allow re-entrant netdev_connect calls */ @@ -847,15 +846,6 @@ static void netdev_connect_failed(struct netdev *netdev, if (connect_cb) connect_cb(netdev, result, &status_or_reason, connect_data); - else if (event_filter) { - /* NETDEV_EVENT_DISCONNECT_BY_SME expects a reason code */ - if (result != NETDEV_RESULT_HANDSHAKE_FAILED) - status_or_reason = MMPDU_REASON_CODE_UNSPECIFIED; - - event_filter(netdev, NETDEV_EVENT_DISCONNECT_BY_SME, - &status_or_reason, - connect_data); - } } static void netdev_connect_failed_cb(struct l_genl_msg *msg, void *user_data) @@ -900,12 +890,42 @@ static void netdev_deauth_and_fail_connection(struct netdev *netdev, netdev_send_and_fail_connection(netdev, result, status_code, msg); } +/* + * If we have a connection callback pending, either through netdev_connect + * or netdev_reassociate, then invoke that callback with the @result and + * @status_or_reason. Otherwise, invoke the event callback with the @event + * and @status_or_reason. + * + * This is useful for situations where handshaking or setting keys somehow + * fails (perhaps due to rekeying), or if the device is removed / brought + * down when keys are being set as a result of a rekey + */ +static void netdev_disconnected(struct netdev *netdev, + enum netdev_result result, + enum netdev_event event, + uint16_t status_or_reason) +{ + netdev_event_func_t event_filter = netdev->event_filter; + void *event_data = netdev->user_data; + + if (netdev->connect_cb) { + netdev_connect_failed(netdev, result, status_or_reason); + return; + } + + netdev_connect_free(netdev); + + if (event_filter) + event_filter(netdev, event, &status_or_reason, event_data); +} + static void netdev_disconnect_by_sme_cb(struct l_genl_msg *msg, void *user_data) { struct netdev *netdev = user_data; netdev->disconnect_cmd_id = 0; - netdev_connect_failed(netdev, netdev->result, netdev->last_code); + netdev_disconnected(netdev, netdev->result, + NETDEV_EVENT_DISCONNECT_BY_SME, netdev->last_code); } static void netdev_disconnect_by_sme(struct netdev *netdev, @@ -1440,7 +1460,8 @@ static void netdev_setting_keys_failed(struct netdev_handshake_state *nhs, * CMD_DISCONNECT */ if (err == -ENETDOWN) { - netdev_connect_failed(netdev, NETDEV_RESULT_ABORTED, + netdev_disconnected(netdev, NETDEV_RESULT_ABORTED, + NETDEV_EVENT_DISCONNECT_BY_SME, MMPDU_STATUS_CODE_UNSPECIFIED); return; }
Commit c59669a366c5 ("netdev: disambiguate between disconnection types") introduced different paths for different types of disconnection notifications from netdev. Formalize this further by having netdev_connect_failed only invoke connect_cb. Disconnections that could be triggered outside of connection related events are now handled on a different code path. For this purpose, netdev_disconnected() is introduced. --- src/netdev.c | 45 +++++++++++++++++++++++++++++++++------------ 1 file changed, 33 insertions(+), 12 deletions(-)